From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by sourceware.org (Postfix) with ESMTPS id 0C0E73857004 for ; Fri, 17 Jul 2020 05:15:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0C0E73857004 Received: by mail-wr1-x42a.google.com with SMTP id z13so9668085wrw.5 for ; Thu, 16 Jul 2020 22:15:49 -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=0zVmI19/x/4cCR2L5V+qFbXo7GqkWX8ZFdUPL+Jowrw=; b=jJHLpQz2b9vXD3KiQmLcd/UDY1PQpZZNyo/b0VEaGivbBmQy6QwjqqWCOVDpmiK6PB AYoabpENm7Se5WXoJoGHVfBycjeVP8vFHO4Ri8thHK9YZqWF+vyle3hfsvHW3F4SQ2X7 W1fcztbxxKHss2Q3geJObUAStVbLuxXcoHzflrlvHOhjc9NtU+rdFFhwVuB4O5rQj4ri lkF2HCuDszmSgUaNCgd78EbRrrtEYVIuPnWh3bGWuzKAQolKqG6Nq+gwNlsCzs0ct84a w46RvDuXumpOaXQMn/pWwQt+UHmauWL81mwA5gh/HlRghYXGbhCw7fL/5qRvkiWNsdQZ W9WA== X-Gm-Message-State: AOAM530rEO5ZiR/Mil8zITzzi6L6ZRg9Kcb0KCpqiuFJFD14VqEcVW6r mwA4n7hbKqQiGQJ+T9WyCICigNIHvmTRLa5tuq0= X-Google-Smtp-Source: ABdhPJz6gIzGcRzy49zhxtUS24gGRGp382Bbq1wvuHRdpj5nW6bVSs4JcFSxSMHBejMWMF5HLFTrxiBT6VtvEzXTJNo= X-Received: by 2002:a5d:6386:: with SMTP id p6mr8342710wru.417.1594962948755; Thu, 16 Jul 2020 22:15:48 -0700 (PDT) MIME-Version: 1.0 References: <20200623152917.1742147-1-skpgkp2@gmail.com> <7c1b0a27-2c43-c17c-feab-5160f076f7b1@redhat.com> In-Reply-To: From: Sunil Pandey Date: Thu, 16 Jul 2020 22:15:37 -0700 Message-ID: Subject: Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237] To: Richard Biener Cc: GCC Patches , "H.J. Lu" , Jason Merrill , "Joseph S. Myers" , Sunil K Pandey X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, HK_RANDOM_ENVFROM, HK_RANDOM_FROM, HTML_MESSAGE, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Fri, 17 Jul 2020 05:15:52 -0000 Any comment on revised patch? At least, in finish_decl, decl global attributes are populated. On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey wrote: > On Sat, Jul 4, 2020 at 9:11 AM Richard Biener > wrote: > > > > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill > wrote: > > >On 6/29/20 5:00 AM, Richard Biener wrote: > > >> 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 > >0x7fffea806000 > > >>>>> 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. > > > > > >cp_finish_decl could work, but layout_decl seems like the right spot; > > >if > > >the problem is that the appropriate flags currently aren't being set in > > > > > >time, can't we fix that? > > > > The first and usually only call to layout_decl is from build_decl which > gets nothing more than the type... But yes, I also initially thought that's > the correct spot but it turns out it isn't. > > I added a new function lower_decl_alignment and invoked from > cp_decl_finish/decl_finish. Attached is revised patch. > > > > >> Note it needs to be a place before the frontends possibly > > >> inspect the alignment of the decl > > >> In C++ constexpr evalualtion might also expose alignment > > >> "early" so we really need a frontend solution here. > > > > > >Yes, we need to know the alignment right after the declaration. > > > > > >Jason > > >