From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 34233 invoked by alias); 3 Sep 2019 12:28:45 -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 34225 invoked by uid 89); 3 Sep 2019 12:28:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*f:sk:dA@mail X-HELO: mail-lf1-f68.google.com Received: from mail-lf1-f68.google.com (HELO mail-lf1-f68.google.com) (209.85.167.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Sep 2019 12:28:44 +0000 Received: by mail-lf1-f68.google.com with SMTP id w67so12724982lff.4 for ; Tue, 03 Sep 2019 05:28:43 -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=xRWHvCIJIGtlGaC/obq5IJHmV1ZYjR6OxQ/VD62vcaI=; b=GkcuaGvb6N3pZJidklc66iwzG81FzF4K1BcVRIrn5w2pBqUsBtYTEZXEcUa2zT8AyR qhRtSIXRyoQDc1LZl7sTv/2ywBbs3LL9t16EF5TmwhR0Ryg2yzJB0gksQMq2eq9AQqEl QlVuMtOY4IqBJ7M+LRGgceHAI/y0g/jPIOR0hEeaeyzyyW4U7IWSDSyc387rYb6SmvLJ g+uLTQeVND7AdV/CYjn8lHcaeIfKBNUfMuZ7Bi0HUHp+IdoideDkFucd8Xin1dpoV+nd q5L9b9FHODD9n2BKMWBdIXep8TZxEMZMbtPBIQlnZlF0BoEZ7FYX6XFmayBf76N1+4Ut /jWQ== MIME-Version: 1.0 References: <20190903115632.12955D80335@oc3748833570.ibm.com> In-Reply-To: <20190903115632.12955D80335@oc3748833570.ibm.com> From: Richard Biener Date: Tue, 03 Sep 2019 12:28: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/msg00110.txt.bz2 On Tue, Sep 3, 2019 at 1:56 PM Ulrich Weigand wrote: > > Richard Biener wrote: > > On Mon, Sep 2, 2019 at 10:35 PM Ulrich Weigand wrote: > > > 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. > > Huh, I was simply going after the comment in front of get_object_alignment_2: > Note that the address (and thus the alignment) computed here is based > on the address to which a symbol resolves, whereas DECL_ALIGN is based > on the address at which an object is actually located. These two > addresses are not always the same. For example, on ARM targets, > the address &foo of a Thumb function foo() has the lowest bit set, > whereas foo() itself starts on an even address. Sure, that is another reason - but you are not passing a FUNCTION_DECL here. > combined with the fact that get_object_alignment_2 actually itself > uses type alignment if we have an actual memory object: > /* When EXP is an actual memory reference then we can use > TYPE_ALIGN of a pointer indirection to derive alignment. > Do so only if get_pointer_alignment_1 did not reveal absolute > alignment knowledge and if using that alignment would > improve the situation. */ > > Is this not correct either, or is the situation there somehow different? We're looking at an address, the above applies to places where we actually dereference it and there we take the type from the type of the access and not from the type of the pointer (or the pointed-to type) since that would be equally wrong. > In any case, I'm not sure I understand your example. Is this supposed > to cover the case where "ptr" is a type with 16 byte alignment, while > __int128 only has 8 byte alignment? Yes. > But in that case I'd assume that > every possible value of "ptr" at runtime must be 16 byte aligned, or > else it wouldn't be a valid value of a variable of that type, right? > So in that case assuming a 16-byte alignment would be correct? If you read the C standards fine-print then yes. But people (or even the C frontend!) hardly get that correct since for example for struct __attribute__((packed)) { int i; } s; void foo () { __builtin_printf ("%p", &s.i); } the C fronted actually creates a int * typed pointer for the ADDR_EXPR (and not an int * variant with 1-byte alignment). And people use int * to pass such pointers everywhere. > > 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. > > I guess I can have a look to do it this way, if necessary. > Is there already some example of a builtin that does that? I think the atomic_* builtins avoid (or should avoid) the issue by only "expanding" to the fixed size vairants if the memory is appropriately aligned and otherwise fall back to _n which doesn't assume any alignment(?) There's internal functions that put alignment into extra args, I'm not aware of user-accessible builtins that do. > > 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. > > We definitely must handle the misaligned case (since the **default** > alignment of __int128 on s390x is misaligned for atomic access). But is it fast (and atomic)? Would going the _n way "work" or do you need a special 8-byte-aligned__int128 path? Richard. > > also looks fishy. So maybe it _is_ an option to simply require > > larger alignment? Maybe add targetm.mode_alignment_for_atomics ()? > > That's why this wouldn't work either -- a default __int128 is not > correctly aligned, we can only assume alignment if it was specifically > specified by the user via an attribute or the like. (But since the > latter is a very common case, it is also important to handle.) > > Bye, > Ulrich > > -- > Dr. Ulrich Weigand > GNU/Linux compilers and toolchain > Ulrich.Weigand@de.ibm.com >