From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 130812 invoked by alias); 3 Sep 2019 10:55:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 130804 invoked by uid 89); 3 Sep 2019 10:55:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=carry, unlikely, fes X-HELO: mail-lj1-f195.google.com Received: from mail-lj1-f195.google.com (HELO mail-lj1-f195.google.com) (209.85.208.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Sep 2019 10:55:47 +0000 Received: by mail-lj1-f195.google.com with SMTP id u14so8979712ljj.11 for ; Tue, 03 Sep 2019 03:55:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2vPfOWfACNqgntL2sQl3sIgKh/xQ6eQjdbuztWbAjWQ=; b=PZGZsPWshOM3KYSHZ42D0L09QCY6Y9/sXWULRsnl1syIlucPMY7g3MUlC1gZ4sPvNC lSO6FE0VDMoY1Dsn6h/1Ov8MFCRCspNnR8gZ0vFY+pXJshd1qzWQMbTBgLrXuDJbEL/H t3oBu6znE6hu/EzAwyvGrmM4EHY7ribWeDdV+hiauliXendL12h3mycRoHTV/FxfgjY8 pEYndG2gW+9lTo+Z4zSVd99zA36ykUVmzz1LF3mbn9oXG1l10e4zRaydLqj6Net2Z6Er amt8/GSIF5NgBb5Or0S7ZYMrF1baB4KzlYeyg6yoATNsEN8i/wrpmxmdfSQCiH5qVrSo vZGg== MIME-Version: 1.0 References: <20190902203520.B4025D802F0@oc3748833570.ibm.com> In-Reply-To: <20190902203520.B4025D802F0@oc3748833570.ibm.com> From: Richard Biener Date: Tue, 03 Sep 2019 10:55:00 -0000 Message-ID: Subject: Re: [PATCH] Use type alignment in get_builtin_sync_mem To: Ulrich Weigand Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-09/txt/msg00102.txt.bz2 On Mon, Sep 2, 2019 at 10:35 PM Ulrich Weigand wrote: > > Hello, > > on s390x the 128-bit integer type is only aligned to 8 bytes by default, > but when lock-free atomic operations can only be performed on objects > aligned to 16 bytes. However, we've noticed that GCC sometimes falls > back to library calls *even if* the object is actually 16 byte aligned, > and GCC could easily know this from type alignment information. > > However, it turns out that get_builtin_sync_mem *ignores* type alignment > data, and only looks at *mode* alignment and pointer alignment data. > This is a problem since mode alignment doesn't know about user-provided > alignment attributes, and while pointer alignment does sometimes know > about those, this usually only happens with optimization on and also > if the optimizers can trace pointer assignments to the final object. > > One important use case where the latter does not happen is with the > C++ atomic classes, because here the object is accessed via the "this" > pointer. Pointer alignment tracking at this point never knows the > final object, and thus we always get a library call *even though* > libstdc++ has actually marked the class type with the correct alignment > atttribute. > > Now one question might be, why does get_pointer_alignment not take > type alignment into account by itself? This appears to be deliberate > to avoid considering numeric pointer values to be aligned when they > are not for target-specific reasons (e.g. the low bit that indicates > Thumb on ARM). It's simply because we throw away type casting on GIMPLE so when you see an aligned __int128 * as argument to __builtin_sync* then there's no guarantee it's actually the type the user used here. The user might have written __buitlin_sync_* ((__int128 *) ptr /* cast away bogus over-alignedness */, ...); and GIMPLE happily throws away the cast. > However, this is not an issue in get_builtin_sync_mem, where we are > actually interested in the alignment of the MEM we're just about to > generate, so it should be fine to check type alignment here. The proper way to carry the information from source to RTL expansion (or earlier GIMPLE) is to add another argument to the builtins specifying alignment which the FEs can derive from the argument type given appropriate semantics of the builtin. Alternatively you could make it simply undefined behavior passing not appropriately aligned memory to those builtins. But I guess we support misaligned cases so that's not an option. > This patch does just that, fixing the issue we've been seeing. > > Tested on s390x-ibm-linux. > > OK for mainline? I don't think the patch is technically correct. Unlikely to break for __int128 I guess but quite possibly for smaller types. Oh, and in this context /* The alignment needs to be at least according to that of the mode. */ also looks fishy. So maybe it _is_ an option to simply require larger alignment? Maybe add targetm.mode_alignment_for_atomics ()? Richard. > Bye, > Ulrich > > > ChangeLog: > > * builtins.c (get_builtin_sync_mem): Respect type alignment. > > testsuite/ChangeLog: > > * gcc.target/s390/md/atomic_exchange-1.c: Do not use -latomic. > (aligned_int128): New data type. > (ae_128_0): Use it instead of __int128 to ensure proper alignment > for atomic accesses. > (ae_128_1): Likewise. > (g128): Likewise. > (main): Likewise. > > Index: gcc/builtins.c > =================================================================== > --- gcc/builtins.c (revision 274142) > +++ gcc/builtins.c (working copy) > @@ -6001,9 +6001,16 @@ > > mem = validize_mem (mem); > > - /* The alignment needs to be at least according to that of the mode. */ > - set_mem_align (mem, MAX (GET_MODE_ALIGNMENT (mode), > - get_pointer_alignment (loc))); > + /* The alignment needs to be at least according to that of the mode. > + Also respect alignment requirements of the type, and alignment > + info that may be deduced from the expression itself. */ > + unsigned int align = GET_MODE_ALIGNMENT (mode); > + if (POINTER_TYPE_P (TREE_TYPE (loc))) > + { > + unsigned int talign = min_align_of_type (TREE_TYPE (TREE_TYPE (loc))); > + align = MAX (align, talign * BITS_PER_UNIT); > + } > + set_mem_align (mem, MAX (align, get_pointer_alignment (loc))); > set_mem_alias_set (mem, ALIAS_SET_MEMORY_BARRIER); > MEM_VOLATILE_P (mem) = 1; > > Index: gcc/testsuite/gcc.target/s390/md/atomic_exchange-1.c > =================================================================== > --- gcc/testsuite/gcc.target/s390/md/atomic_exchange-1.c (revision 274142) > +++ gcc/testsuite/gcc.target/s390/md/atomic_exchange-1.c (working copy) > @@ -1,7 +1,7 @@ > /* Machine description pattern tests. */ > > /* { dg-do compile } */ > -/* { dg-options "-lpthread -latomic" } */ > +/* { dg-options "-lpthread" } */ > /* { dg-do run { target { s390_useable_hw } } } */ > > /**/ > @@ -119,19 +119,21 @@ > /**/ > > #ifdef __s390x__ > +typedef __int128 __attribute__((aligned(16))) aligned_int128; > + > __int128 > -ae_128_0 (__int128 *lock) > +ae_128_0 (aligned_int128 *lock) > { > return __atomic_exchange_n (lock, 0, 2); > } > > __int128 > -ae_128_1 (__int128 *lock) > +ae_128_1 (aligned_int128 *lock) > { > return __atomic_exchange_n (lock, 1, 2); > } > > -__int128 g128; > +aligned_int128 g128; > > __int128 > ae_128_g_0 (void) > @@ -274,7 +276,7 @@ > > #ifdef __s390x__ > { > - __int128 lock; > + aligned_int128 lock; > __int128 rval; > > lock = oval; > -- > Dr. Ulrich Weigand > GNU/Linux compilers and toolchain > Ulrich.Weigand@de.ibm.com >