From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by sourceware.org (Postfix) with ESMTPS id 655E3387085C for ; Mon, 29 Jun 2020 09:39:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 655E3387085C Received: by mail-ed1-x531.google.com with SMTP id b15so12241110edy.7 for ; Mon, 29 Jun 2020 02:39:29 -0700 (PDT) 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=w/oXhlx2Udb0Ynzi2QzGCM9tPjGaWTS71J8oc9mgc4s=; b=II9oQBBYti8XUZyO1fa5Cvok6QPmsTQmXAviV2fEpW6tmWRsccVoxj2Nb0SBjE3shc rIlousm4d19Yxk7x+2HPJGfTFXE8Em5682q4A6lZzgY3BfGRz5qqmcaBdpjG/9yja/7L 3TpLa+6h5rekkwz/goqHwmIbUjRPKRMZ2CY8Slxl1HjiavwdKEg7kSXaAl5UVIl+lgfd NIhgMSCOLegaiNw+zw2e9UHZysmrlNZkWjBvyCeGfL5BuTW0g/Z2I0XhJLvIvUOpMQAq OZ2IEoeZR61re6JKvlzQeUDjlpHIo8jLTdtUb8I6bLmn2AqolT5JmnvhPfpkBhfVe3oT 3ooA== X-Gm-Message-State: AOAM533buMZWWyc5hX2d5G5BFLZ4cRLkmbsstHdeTgom5zt5kgDHNdv9 MV2btb85hroSf5sCtly6XeV3Q6WP7LKTnKi3P/KMCLE+ X-Google-Smtp-Source: ABdhPJw+brdfuzX7W/+ZJmwl+6m4aSs+K/32+ZzJRNUJhHSPy1Y2weRxqXJT5tnQRXP1k/0TrXDu3xS5KQrB0iG+Njc= X-Received: by 2002:a17:906:17c8:: with SMTP id u8mr13289813eje.129.1593421235628; Mon, 29 Jun 2020 02:00:35 -0700 (PDT) MIME-Version: 1.0 References: <20200623152917.1742147-1-skpgkp2@gmail.com> In-Reply-To: From: Richard Biener Date: Mon, 29 Jun 2020 11:00:24 +0200 Message-ID: Subject: Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237] To: "H.J. Lu" Cc: Sunil Pandey , "Joseph S. Myers" , Sunil K Pandey , GCC Patches , Jason Merrill Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Jun 2020 09:39:31 -0000 On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu wrote: > > On Thu, Jun 25, 2020 at 1:10 AM Richard Biener > wrote: > > > > On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey wrote: > > > > > > On Wed, Jun 24, 2020 at 12:30 AM Richard Biener > > > wrote: > > > > > > > > On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches > > > > wrote: > > > > > > > > > > From: Sunil K Pandey > > > > > > > > > > Default for this hook is NOP. For x86, in 32 bit mode, this hook > > > > > sets alignment of long long on stack to 32 bits if preferred stack > > > > > boundary is 32 bits. > > > > > > > > > > - This patch fixes > > > > > gcc.target/i386/pr69454-2.c > > > > > gcc.target/i386/stackalign/longlong-1.c > > > > > - Regression test on x86-64, no new fail introduced. > > > > > > > > I think the name is badly chosen, TARGET_LOWER_LOCAL_DECL_ALIGNMENT > > > > > > Yes, I can change the target hook name. > > > > > > > would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to be > > > > renamed to INCREASE_LOCAL_DECL_ALIGNMENT). > > > > > > It seems like LOCAL_DECL_ALIGNMENT macro documentation is incorrect. > > > It increases as well as decreases alignment based on condition(-m32 > > > -mpreferred-stack-boundary=2) > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95885 > > > > > > > > > > > You're calling it from do_type_align which IMHO is dangerous since that's > > > > invoked from FIELD_DECL layout as well. Instead invoke it from > > > > layout_decl itself where we do > > > > > > > > if (code != FIELD_DECL) > > > > /* For non-fields, update the alignment from the type. */ > > > > do_type_align (type, decl); > > > > > > > > and invoke the hook _after_ do_type_align. Also avoid > > > > invoking the hook on globals or hard regs and only > > > > invoke it on VAR_DECLs, thus only > > > > > > > > if (VAR_P (decl) && !is_global_var (decl) && !DECL_HARD_REGISTER (decl)) > > > > > > It seems like decl property is not fully populated at this point call > > > to is_global_var (decl) on global variable return false. > > > > > > $ cat foo.c > > > long long x; > > > int main() > > > { > > > if (__alignof__(x) != 8) > > > __builtin_abort(); > > > return 0; > > > } > > > > > > Breakpoint 1, layout_decl (decl=0x7ffff7ffbb40, known_align=0) > > > at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout.c:674 > > > 674 do_type_align (type, decl); > > > Missing separate debuginfos, use: dnf debuginfo-install > > > gmp-6.1.2-10.fc31.x86_64 isl-0.16.1-9.fc31.x86_64 > > > libmpc-1.1.0-4.fc31.x86_64 mpfr-3.1.6-5.fc31.x86_64 > > > zlib-1.2.11-20.fc31.x86_64 > > > (gdb) call debug_tree(decl) > > > > > type > > size > > > unit-size > > > align:64 warn_if_not_align:0 symtab:0 alias-set -1 > > > canonical-type 0x7fffea801888 precision:64 min > > 0x7fffea7e8fd8 -9223372036854775808> max > > 9223372036854775807> > > > pointer_to_this > > > > DI foo.c:1:11 size unit-size > > > > > > align:1 warn_if_not_align:0> > > > > > > (gdb) p is_global_var(decl) > > > $1 = false > > > (gdb) > > > > > > > > > What about calling hook here > > > > > > 603 do_type_align (tree type, tree decl) > > > 604 { > > > 605 if (TYPE_ALIGN (type) > DECL_ALIGN (decl)) > > > 606 { > > > 607 SET_DECL_ALIGN (decl, TYPE_ALIGN (type)); > > > 608 if (TREE_CODE (decl) == FIELD_DECL) > > > 609 DECL_USER_ALIGN (decl) = TYPE_USER_ALIGN (type); > > > 610 else > > > 611 /* Lower local decl alignment */ > > > 612 if (VAR_P (decl) > > > 613 && !is_global_var (decl) > > > 614 && !DECL_HARD_REGISTER (decl) > > > 615 && cfun != NULL) > > > 616 targetm.lower_local_decl_alignment (decl); > > > 617 } > > > > But that doesn't change anything (obviously). layout_decl > > is called quite early, too early it looks like. > > > > Now there doesn't seem to be any other good place where > > we are sure to catch the decl before we evaluate things > > like __alignof__ > > > > void __attribute__((noipa)) > > foo (__SIZE_TYPE__ align, long long *p) > > { > > if ((__SIZE_TYPE__)p & (align-1)) > > __builtin_abort (); > > } > > int main() > > { > > long long y; > > foo (_Alignof y, &y); > > return 0; > > } > > > > Joseph/Jason - do you have a good recommendation > > how to deal with targets where natural alignment > > is supposed to be lowered for optimization purposes? > > (this case is for i?86 to avoid dynamic stack re-alignment > > to align long long to 8 bytes with -mpreferred-stack-boundary=2) > > > > I note that for -mincoming-stack-boundary=2 we do perform > > dynamic stack re-alignment already. > > > > I can't find a suitable existing target macro/hook for this, > > but my gut feeling is that the default alignment should > > instead be the lower one and instead the alignment for > > globals should be raised as optimization? > > > > Here is the updated patch from Sunil. It does not address the fundamental issue that during do_type_align the is_global_var predicate is not reliable. This means that for int main() { extern long z; } the new hook (with -m32 -mpreferred-stack-boundary=2) will lower the alignment of 'z' which looks wrong. During layout_decl we can unfortunately not distinguish between locals and globals. We need to find another spot to adjust alignment of locals. For C that might be in finish_decl, for C++ there's probably another suitable place. Note it needs to be a place before the frontends possibly inspect the alignment of the decl - I hope there's no self-reflective way of using it like void foo() { long a[__alignof__(this)]; } with 'this' refering to the actual declarator. But you never know C++ ... In C++ constexpr evalualtion might also expose alignment "early" so we really need a frontend solution here. My limited C++ fu would come up with sth like constexpr int a = []() { long x; return __alignof__(x); }; or so. I guess even local templates might expose alignment? void foo() { long a; template struct X; template <> struct X<4> {}; X<__alignof__(a)> b; } and eventually the alignof might be even dependent. Richard. > -- > H.J.