From owner-dev-commits-src-main@freebsd.org Wed Feb 17 21:01:19 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id C048D532C1D; Wed, 17 Feb 2021 21:01:19 +0000 (UTC) (envelope-from arichardson.kde@gmail.com) Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Dgqy74xR8z3NxC; Wed, 17 Feb 2021 21:01:19 +0000 (UTC) (envelope-from arichardson.kde@gmail.com) Received: by mail-ej1-f47.google.com with SMTP id d8so11447967ejc.4; Wed, 17 Feb 2021 13:01:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=hNWAUYw4bNtQpFWGNJx557HzSo6foXDFS1pTJATUoS0=; b=FTRvOEdeICFo6cb8nTWrox6qfRJwJrjoiP5qIxg63U5vKRD8AwS7gYcHue9UdjRyC7 TU1zAJ9G52nnn4eSSsYiFz7OhW8JSis/YByc4/r2Xn9O4z4vf5XIKgoK5pjvkoUYkGrV kTRy7dNnMyzJLQIfyKw0QGKbM0tHBc24XcTpaZ0H9xkQXhDbFHaggp3AYWN0jFelrn9B iC7eojbq5tuLmczHKnpl3Xb/RbAMJnJivs1te282Izp9PyGxSmXcEuUTI4TWcQolLABG IrNfvwEUZqmdFskZ0YUnJx1PF0kksd0l9M2e0j7koVfiqk9ooXJ10xJD+tlaUJvME0KD ViIg== X-Gm-Message-State: AOAM532Q+BUWE0i1JvjONcIjnnBPn7SNFAQSIoFXBxBrD0PucQ3XgbOq kMBslVcDA4P+d18JipRLHWBbs3i4AJQ= X-Google-Smtp-Source: ABdhPJyTKQE6nKJC0J+rzlLyP3ynTJ/JFjE6sIwrm9wLgsmNN97N3AFaCBEA/fxO7ah1hX+a5O3gbQ== X-Received: by 2002:a17:906:2358:: with SMTP id m24mr771265eja.333.1613595677613; Wed, 17 Feb 2021 13:01:17 -0800 (PST) Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com. [209.85.221.45]) by smtp.gmail.com with ESMTPSA id g20sm1591039ejz.54.2021.02.17.13.01.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Feb 2021 13:01:17 -0800 (PST) Received: by mail-wr1-f45.google.com with SMTP id u14so18846971wri.3; Wed, 17 Feb 2021 13:01:17 -0800 (PST) X-Received: by 2002:a5d:444a:: with SMTP id x10mr1001394wrr.409.1613595676783; Wed, 17 Feb 2021 13:01:16 -0800 (PST) MIME-Version: 1.0 References: <202102031604.113G4SQq019037@gitrepo.freebsd.org> In-Reply-To: From: Alexander Richardson Date: Wed, 17 Feb 2021 21:01:05 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: git: 8fa6abb6f4f6 - main - Expose clang's alignment builtins and use them for roundup2/rounddown2 To: Ryan Libby , John Baldwin Cc: src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4Dgqy74xR8z3NxC X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; TAGGED_FROM(0.00)[]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Feb 2021 21:01:19 -0000 On Wed, 17 Feb 2021 at 20:46, Ryan Libby wrote: > > On Wed, Feb 3, 2021 at 8:04 AM Alex Richardson wrote: > > > > The branch main has been updated by arichardson: > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=8fa6abb6f4f64f4f23e2920e2aea7996566851a4 > > > > commit 8fa6abb6f4f64f4f23e2920e2aea7996566851a4 > > Author: Alex Richardson > > AuthorDate: 2021-02-03 15:27:17 +0000 > > Commit: Alex Richardson > > CommitDate: 2021-02-03 16:02:54 +0000 > > > > Expose clang's alignment builtins and use them for roundup2/rounddown2 > > > > This makes roundup2/rounddown2 type- and const-preserving and allows > > using it on pointer types without casting to uintptr_t first. Not > > performing pointer-to-integer conversions also helps the compiler's > > optimization passes and can therefore result in better code generation. > > When using it with integer values there should be no change other than > > the compiler checking that the alignment value is a valid power-of-two. > > > > I originally implemented these builtins for CHERI a few years ago and > > they have been very useful for CheriBSD. However, they are also useful > > for non-CHERI code so I was able to upstream them for Clang 10.0. > > > > Rationale from the clang documentation: > > Clang provides builtins to support checking and adjusting alignment > > of pointers and integers. These builtins can be used to avoid relying > > on implementation-defined behavior of arithmetic on integers derived > > from pointers. Additionally, these builtins retain type information > > and, unlike bitwise arithmetic, they can perform semantic checking on > > the alignment value. > > > > There is also a feature request for GCC, so GCC may also support it in > > the future: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98641 > > > > Reviewed By: brooks, jhb, imp > > Differential Revision: https://reviews.freebsd.org/D28332 > > --- > > sys/sys/cdefs.h | 19 +++++++++++++++++++ > > sys/sys/param.h | 4 ++-- > > 2 files changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/sys/sys/cdefs.h b/sys/sys/cdefs.h > > index 75bedd4b8128..72ef942084f2 100644 > > --- a/sys/sys/cdefs.h > > +++ b/sys/sys/cdefs.h > > @@ -884,4 +884,23 @@ > > #define __guarded_by(x) __lock_annotate(guarded_by(x)) > > #define __pt_guarded_by(x) __lock_annotate(pt_guarded_by(x)) > > > > +/* Alignment builtins for better type checking and improved code generation. */ > > +/* Provide fallback versions for other compilers (GCC/Clang < 10): */ > > +#if !__has_builtin(__builtin_is_aligned) > > +#define __builtin_is_aligned(x, align) \ > > + (((__uintptr_t)x & ((align) - 1)) == 0) > > +#endif > > +#if !__has_builtin(__builtin_align_up) > > +#define __builtin_align_up(x, align) \ > > + ((__typeof__(x))(((__uintptr_t)(x)+((align)-1))&(~((align)-1)))) > > +#endif > > +#if !__has_builtin(__builtin_align_down) > > +#define __builtin_align_down(x, align) \ > > + ((__typeof__(x))((x)&(~((align)-1)))) > > +#endif > > + > > +#define __align_up(x, y) __builtin_align_up(x, y) > > +#define __align_down(x, y) __builtin_align_down(x, y) > > +#define __is_aligned(x, y) __builtin_is_aligned(x, y) > > + > > Since these are only valid for powers of 2, I think it would be good to > indicate that in the names (__align_up2() etc). > > > #endif /* !_SYS_CDEFS_H_ */ > > diff --git a/sys/sys/param.h b/sys/sys/param.h > > index 079357a19d47..d6f1eb21dcd2 100644 > > --- a/sys/sys/param.h > > +++ b/sys/sys/param.h > > @@ -305,9 +305,9 @@ > > #endif > > #define nitems(x) (sizeof((x)) / sizeof((x)[0])) > > #define rounddown(x, y) (((x)/(y))*(y)) > > -#define rounddown2(x, y) ((x)&(~((y)-1))) /* if y is power of two */ > > +#define rounddown2(x, y) __align_down(x, y) /* if y is power of two */ > > #define roundup(x, y) ((((x)+((y)-1))/(y))*(y)) /* to any y */ > > -#define roundup2(x, y) (((x)+((y)-1))&(~((y)-1))) /* if y is powers of two */ > > +#define roundup2(x, y) __align_up(x, y) /* if y is powers of two */ > > #define powerof2(x) ((((x)-1)&(x))==0) > > > > /* Macros for min/max. */ > > This broke the gcc build: > https://ci.freebsd.org/job/FreeBSD-main-amd64-gcc6_build/3200/console > 00:40:30 --- all_subdir_firewire --- > 00:40:30 In file included from /workspace/src/sys/sys/types.h:43:0, > 00:40:30 from /workspace/src/sys/sys/param.h:99, > 00:40:30 from /workspace/src/sys/dev/firewire/fwohci.c:40: > 00:40:30 /workspace/src/sys/dev/firewire/fwohci.c: In function > 'fwohci_get_plen': > 00:40:30 /workspace/src/sys/dev/firewire/fwohci.c:2699:17: error: > 'typeof' applied to a bit-field > 00:40:30 r += roundup2(fp->mode.wreqb.len, sizeof(uint32_t)); > > We could: > - Drop the cast for the fallback. > - Cast with (__typeof__(+(x))) which unfortunately promotes e.g. char > to int but otherwise I think behaves okay, and the promotion was > previously happening for roundup2/rounddown2 anyway. > - Punt the casting burden to callers of roundup2/rounddown2. > > Ryan Hi Ryan, https://reviews.freebsd.org/D28599 fixes this specific issue. One reason for the typeof() is to allow using it with pointer types. Currently there are no such uses, but we have some in CheriBSD. Unfortunately +(x) would break that. (x)+0 should work for most cases, but does break void*. I believe the current approach only breaks with bitfields (which is hopefully rare), so I think adding casts for GCC when roundup2() in those cases might be the simplest solution. Thanks, Alex