From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x541.google.com (mail-ed1-x541.google.com [IPv6:2a00:1450:4864:20::541]) by sourceware.org (Postfix) with ESMTPS id 011D63844041 for ; Sat, 4 Jul 2020 16:11:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 011D63844041 Received: by mail-ed1-x541.google.com with SMTP id g20so30318226edm.4 for ; Sat, 04 Jul 2020 09:11:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:user-agent:in-reply-to:references :mime-version:content-transfer-encoding:subject:to:cc:from :message-id; bh=hqLeoMRfIYJCCgwK0JgllX67z9mQCJLg/89V4mglD34=; b=edBI9F6nDZpgP5woMBNW/bYIp4SvvLT7zMEdX2FRksitM4+qHOlfbk+Ge9I3GP4TZj 0cXLuIdJQBHDG0eN7SEKKD7hsQfuGFiigKKWgrTE4CHdI5kzQ2Q2n7sNoKITI4QxOaDy 2F2PVCeIEfzt5bF+KDmLcSo7ZbYCxc5NKKJqsquEjqps/QUqYkHnlkgn1aOH574V9S65 jE9+4zAVQE5A2a/kLiYJQhVTRNixC6M6JMMoRxihdlrIXLb/d32GgHbw6sCDELK7MT6z BwUTI89cC2++U6TvsGI6XfN+MoVE46/Xjuh/FP0nfeD8OFFgDm5YvsWoEMqpI6AmQap+ qoXw== X-Gm-Message-State: AOAM531lxOblVoRnXFUPrGyI6FwZ67LSBmX/kYnkyXlHMHhpnuJ3waKa wBLBSgydC6mCLQwra60Wkcw= X-Google-Smtp-Source: ABdhPJycVz2Piyfv4yTQ6qeNgCcSJ/uP8bks08BjRTVLLvfDFTEFBhCO98mukcB/N5xE/j29vGa0wg== X-Received: by 2002:a50:ce45:: with SMTP id k5mr47245316edj.80.1593879103067; Sat, 04 Jul 2020 09:11:43 -0700 (PDT) Received: from [192.168.178.32] (x5f76be54.dyn.telefonica.de. [95.118.190.84]) by smtp.gmail.com with ESMTPSA id s7sm16132124edr.57.2020.07.04.09.11.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 04 Jul 2020 09:11:42 -0700 (PDT) Date: Sat, 04 Jul 2020 18:11:41 +0200 User-Agent: K-9 Mail for Android In-Reply-To: <7c1b0a27-2c43-c17c-feab-5160f076f7b1@redhat.com> References: <20200623152917.1742147-1-skpgkp2@gmail.com> <7c1b0a27-2c43-c17c-feab-5160f076f7b1@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237] To: Jason Merrill ,"H.J. Lu" CC: Sunil Pandey , "Joseph S. Myers" , Sunil K Pandey , GCC Patches From: Richard Biener Message-ID: X-Spam-Status: No, score=-3.8 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: Sat, 04 Jul 2020 16:11:47 -0000 On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill w= rote: >On 6/29/20 5:00 AM, Richard Biener wrote: >> On Fri, Jun 26, 2020 at 10:11 PM H=2EJ=2E 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=2E 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=2E >>>>>>> >>>>>>> - This patch fixes >>>>>>> gcc=2Etarget/i386/pr69454-2=2Ec >>>>>>> gcc=2Etarget/i386/stackalign/longlong-1=2Ec >>>>>>> - Regression test on x86-64, no new fail introduced=2E >>>>>> >>>>>> I think the name is badly chosen, >TARGET_LOWER_LOCAL_DECL_ALIGNMENT >>>>> >>>>> Yes, I can change the target hook name=2E >>>>> >>>>>> would be better suited (and then asks for LOCAL_DECL_ALIGNMENT to >be >>>>>> renamed to INCREASE_LOCAL_DECL_ALIGNMENT)=2E >>>>> >>>>> It seems like LOCAL_DECL_ALIGNMENT macro documentation is >incorrect=2E >>>>> It increases as well as decreases alignment based on >condition(-m32 >>>>> -mpreferred-stack-boundary=3D2) >>>>> https://gcc=2Egnu=2Eorg/bugzilla/show_bug=2Ecgi?id=3D95885 >>>>> >>>>>> >>>>>> You're calling it from do_type_align which IMHO is dangerous >since that's >>>>>> invoked from FIELD_DECL layout as well=2E Instead invoke it from >>>>>> layout_decl itself where we do >>>>>> >>>>>> if (code !=3D FIELD_DECL) >>>>>> /* For non-fields, update the alignment from the type=2E */ >>>>>> do_type_align (type, decl); >>>>>> >>>>>> and invoke the hook _after_ do_type_align=2E 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=2E >>>>> >>>>> $ cat foo=2Ec >>>>> long long x; >>>>> int main() >>>>> { >>>>> if (__alignof__(x) !=3D 8) >>>>> __builtin_abort(); >>>>> return 0; >>>>> } >>>>> >>>>> Breakpoint 1, layout_decl (decl=3D0x7ffff7ffbb40, known_align=3D0) >>>>> at /local/skpandey/gccwork/gccwork/gcc/gcc/stor-layout=2Ec:674 >>>>> 674 do_type_align (type, decl); >>>>> Missing separate debuginfos, use: dnf debuginfo-install >>>>> gmp-6=2E1=2E2-10=2Efc31=2Ex86_64 isl-0=2E16=2E1-9=2Efc31=2Ex86_64 >>>>> libmpc-1=2E1=2E0-4=2Efc31=2Ex86_64 mpfr-3=2E1=2E6-5=2Efc31=2Ex86_64 >>>>> zlib-1=2E2=2E11-20=2Efc31=2Ex86_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=2Ec:1:11 size unit-size >>>>> >>>>> align:1 warn_if_not_align:0> >>>>> >>>>> (gdb) p is_global_var(decl) >>>>> $1 =3D 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) =3D=3D FIELD_DECL) >>>>> 609 DECL_USER_ALIGN (decl) =3D 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 !=3D NULL) >>>>> 616 targetm=2Elower_local_decl_alignment (decl); >>>>> 617 } >>>> >>>> But that doesn't change anything (obviously)=2E layout_decl >>>> is called quite early, too early it looks like=2E >>>> >>>> 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=3D2) >>>> >>>> I note that for -mincoming-stack-boundary=3D2 we do perform >>>> dynamic stack re-alignment already=2E >>>> >>>> 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=2E >>=20 >> It does not address the fundamental issue that during >> do_type_align the is_global_var predicate is not >> reliable=2E This means that for >>=20 >> int main() >> { >> extern long z; >> } >>=20 >> the new hook (with -m32 -mpreferred-stack-boundary=3D2) >> will lower the alignment of 'z' which looks wrong=2E During >> layout_decl we can unfortunately not distinguish between >> locals and globals=2E We need to find another spot to adjust >> alignment of locals=2E For C that might be in finish_decl, >> for C++ there's probably another suitable place=2E > >cp_finish_decl could work, but layout_decl seems like the right spot; >if=20 >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 ge= ts nothing more than the type=2E=2E=2E But yes, I also initially thought th= at's the correct spot but it turns out it isn't=2E=20 >> 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=2E > >Yes, we need to know the alignment right after the declaration=2E > >Jason