From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x143.google.com (mail-il1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) by sourceware.org (Postfix) with ESMTPS id 2CF0F3857033 for ; Wed, 22 Jul 2020 14:38:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2CF0F3857033 Received: by mail-il1-x143.google.com with SMTP id q3so1473662ilt.8 for ; Wed, 22 Jul 2020 07:38:26 -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=CQHmk9oNINl86I8axZ0BwPQeC1BNBKKzxLI2F/g7wpg=; b=M28iYfOVEDpfO6wKjI/5KWYdT9u9NUml8t0kp8ePB9A3z67qZp+gspiApSe8FpdPA8 fTnzCiekIZKDLa5xqb8G+Fn5ZSxMeXTf12yxcjfLVtkhbRA10BIZ2l1U3kbqQCT8NjfR zac76cS9RpxuQStRwIk7+NSokuWqXi1QvBWgHsPEXvPCOWxUz7pOuJjP80ME2v4zL9lK 7z3k6xv18dtmU/SrpNUaDTdzlMEkK8dFuFK/ZvP1TzMtCP4THMdLvqG7aw5d/hBUSNDz 3hBc8vVwpfPLcWno97e7twKiNUFuSMW1SY7atgYRosv8JUFwY9jQxszpJOWF8Wj9NQgP RShQ== X-Gm-Message-State: AOAM531Bo3u9CKDSFfH8wzpqns4b8zOR1ZwlGc2kyl5GMuEo+SLYoxoO ciYptrIlFlKd9Nzm/ooeV8n3FSQGqroV8OGoZ8M= X-Google-Smtp-Source: ABdhPJx/05LQOA/tLor5nzqOo8LR6UEF2cHlMlbmIiTOZzYhoAC/T9H18iYfukV9IJcIM6a9uqpimfB8LyCGmLC+EuY= X-Received: by 2002:a92:d3c7:: with SMTP id c7mr211699ilh.292.1595428705526; Wed, 22 Jul 2020 07:38:25 -0700 (PDT) MIME-Version: 1.0 References: <20200623152917.1742147-1-skpgkp2@gmail.com> <2145070.GYPV22okft@tpdeb> In-Reply-To: <2145070.GYPV22okft@tpdeb> From: "H.J. Lu" Date: Wed, 22 Jul 2020 07:37:49 -0700 Message-ID: Subject: Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237] To: Dimitar Dimitrov Cc: GCC Patches , Sunil Pandey , Jakub Jelinek Content-Type: multipart/mixed; boundary="000000000000420da405ab08b160" X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, 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: Wed, 22 Jul 2020 14:38:28 -0000 --000000000000420da405ab08b160 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jul 22, 2020 at 7:25 AM Dimitar Dimitrov wrote: > > On =D1=81=D1=80=D1=8F=D0=B4=D0=B0, 22 =D1=8E=D0=BB=D0=B8 2020 =D0=B3. 2:0= 4:35 EEST Sunil Pandey via Gcc-patches wrote: > > On Tue, Jul 21, 2020 at 12:50 AM Richard Biener > > > > wrote: > > > On Tue, Jul 21, 2020 at 7:16 AM Sunil Pandey wrot= e: > > > > On Mon, Jul 20, 2020 at 5:06 AM Richard Biener > > > > > > > > wrote: > > > > > On Sat, Jul 18, 2020 at 7:57 AM Sunil Pandey > wrote: > > > > > > On Fri, Jul 17, 2020 at 1:22 AM Richard Biener > > > > > > > > > > > > wrote: > > > > > > > On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey > wrote: > > > > > > > > Any comment on revised patch? At least, in finish_decl, de= cl > > > > > > > > global attributes are populated.> > > > > > > > > > > > +static void > > > > > > > +ix86_lower_local_decl_alignment (tree decl) > > > > > > > +{ > > > > > > > + unsigned new_align =3D LOCAL_DECL_ALIGNMENT (decl); > > > > > > > > > > > > > > please use the macro-expanded call here since we want to amen= d > > > > > > > ix86_local_alignment to _not_ return a lower alignment when > > > > > > > called as LOCAL_DECL_ALIGNMENT (by adding a new parameter > > > > > > > to ix86_local_alignment). Can you also amend the patch in th= is > > > > > > > way? > > > > > > > > > > > > > > + if (new_align < DECL_ALIGN (decl)) > > > > > > > + SET_DECL_ALIGN (decl, new_align); > > > > > > > > > > > > > > diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c > > > > > > > index 81bd2ee94f0..1ae99e30ed1 100644 > > > > > > > --- a/gcc/c/c-decl.c > > > > > > > +++ b/gcc/c/c-decl.c > > > > > > > @@ -5601,6 +5601,8 @@ finish_decl (tree decl, location_t init= _loc, > > > > > > > tree init,> > > > > > > > > > > > } > > > > > > > > > > > > > > invoke_plugin_callbacks (PLUGIN_FINISH_DECL, decl); > > > > > > > > > > > > > > + /* Lower local decl alignment. */ > > > > > > > + lower_decl_alignment (decl); > > > > > > > > > > > > > > } > > > > > > > > > > > > > > should come before plugin hook invocation, likewise for the > > > > > > > cp_finish_decl case. > > > > > > > > > > > > > > +/* Lower DECL alignment. */ > > > > > > > + > > > > > > > +void > > > > > > > +lower_decl_alignment (tree decl) > > > > > > > +{ > > > > > > > + if (VAR_P (decl) > > > > > > > + && !is_global_var (decl) > > > > > > > + && !DECL_HARD_REGISTER (decl)) > > > > > > > + targetm.lower_local_decl_alignment (decl); > > > > > > > +} > > > > > > > > > > > > > > please avoid this function, it's name sounds too generic and = it's > > > > > > > not worth > > > > > > > adding a public API for two calls. > > > > > > > > > > > > > > Alltogether this should avoid the x86 issue leaving left-over= s > > > > > > > (your identified inliner case) as missed optimization [for th= e > > > > > > > linux kernel which appearantly decided that > > > > > > > -mpreferred-stack-boundary=3D2 is a good ABI to use]. > > > > > > > > > > > > > > Richard. > > > > > > > > > > > > Revised patch attached. > > > > > > > > > > @@ -16776,7 +16783,7 @@ ix86_data_alignment (tree type, unsigned = int > > > > > align, bool opt) > > > > > > > > > > unsigned int > > > > > ix86_local_alignment (tree exp, machine_mode mode, > > > > > > > > > > - unsigned int align) > > > > > + unsigned int align, bool setalign) > > > > > > > > > > { > > > > > > > > > > tree type, decl; > > > > > > > > > > @@ -16801,6 +16808,10 @@ ix86_local_alignment (tree exp, machine_= mode > > > > > mode, > > > > > > > > > > && (!decl || !DECL_USER_ALIGN (decl))) > > > > > > > > > > align =3D 32; > > > > > > > > > > + /* Lower decl alignment. */ > > > > > + if (setalign && align < DECL_ALIGN (decl)) > > > > > + SET_DECL_ALIGN (decl, align); > > > > > + > > > > > > > > > > /* If TYPE is NULL, we are allocating a stack slot for caller-= save > > > > > > > > > > register in MODE. We will return the largest alignment of = XF > > > > > and DF. */ > > > > > > > > > > sorry for not being clear - the parameter should indicate whether= an > > > > > alignment lower > > > > > than natural alignment is OK to return thus sth like > > > > > > > > > > diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c > > > > > index 31757b044c8..19703cbceb9 100644 > > > > > --- a/gcc/config/i386/i386.c > > > > > +++ b/gcc/config/i386/i386.c > > > > > @@ -16641,7 +16641,7 @@ ix86_data_alignment (tree type, unsigned = int > > > > > align, bool opt) > > > > > > > > > > unsigned int > > > > > ix86_local_alignment (tree exp, machine_mode mode, > > > > > > > > > > - unsigned int align) > > > > > + unsigned int align, bool may_lower) > > > > > > > > > > { > > > > > > > > > > tree type, decl; > > > > > > > > > > @@ -16658,7 +16658,8 @@ ix86_local_alignment (tree exp, machine_m= ode > > > > > mode, > > > > > > > > > > /* Don't do dynamic stack realignment for long long objects wi= th > > > > > > > > > > -mpreferred-stack-boundary=3D2. */ > > > > > > > > > > - if (!TARGET_64BIT > > > > > + if (may_lower > > > > > + && !TARGET_64BIT > > > > > > > > > > && align =3D=3D 64 > > > > > && ix86_preferred_stack_boundary < 64 > > > > > && (mode =3D=3D DImode || (type && TYPE_MODE (type) =3D=3D= DImode)) > > > > > > > > > > I also believe that spill_slot_alignment () should be able to get= the > > > > > lower alignment > > > > > for long long but not get_stack_local_alignment () (both use > > > > > STACK_SLOT_ALIGNMENT). > > > > > Some uses of STACK_SLOT_ALIGNMENT also look fishy with respect to= mem > > > > > attributes and alignment. > > > > > > > > > > Otherwise the patch looks reasonable to salvage a misguided > > > > > optimization for a non-standard ABI. If it is sufficient to make= the > > > > > people using that ABI happy is of course another question. I'd > > > > > rather see them stop using it ... > > > > > > > > > > That said, I'm hesitant to be the only one OKing this ugliness bu= t I'd > > > > > immediately > > > > > OK a patch removing the questionable hunk from ix86_local_alignme= nt ;) > > > > > > > > > > Jakub, Jeff - any opinion? > > > > > > > > > > Richard. > > > > > > > > Revised patch attached. > > > > > > You are now passing 'true' to ix86_local_alignment for all callers, > > > that's not correct. > > > I said at most STACK_SLOT_ALIGNMENT _might_ be able to take the lower > > > alignment but some callers look suspicious so I wasn't sure. > > > > > > Which means - please pass false for LOCAL_ALIGNMENT, STACK_SLOT_ALIGN= MENT > > > and LOCAL_DECL_ALIGNMENT. > > > > > > + /* Lower local decl alignment. */ > > > + > > > + if (VAR_P (decl) > > > + && !is_global_var (decl) > > > + && !DECL_HARD_REGISTER (decl)) > > > + targetm.lower_local_decl_alignment (decl); > > > > > > the comment is quite useless, just repeating what the code does. > > > Please rephrase it > > > as > > > > > > /* This is the last point we can lower alignment so give the target= the > > > > > > chance to do so. */ > > > > > > and remove the vertical space after the comment. > > > > > > OK with those changes. > > > Richard. > > > > Updated patch attached. I will ask H.J. to check it in for me. > > --Sunil > > Hi Sunil, > > I get regressions for PRU and AVR: > NA->UNRESOLVED: c-c++-common/pr95237-6.c -std=3Dgnu++14 execution test > > Is pr95237-6.c missing an ia32 target filter? I am checking in this to limit it to x86 targets as an obvious fix. > /* { dg-require-effective-target ia32 } */ > > If all pr95237-*.c cases require target ia32, then shouldn't they be move= d to > gcc.target/i386/ ? > They are both C and C++ tests. gcc.target/i386/ is C only. --=20 H.J. --000000000000420da405ab08b160 Content-Type: text/x-patch; charset="US-ASCII"; name="0001-Limit-pr95237-6.c-to-x86-targets.patch" Content-Disposition: attachment; filename="0001-Limit-pr95237-6.c-to-x86-targets.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kcxgzftf0 RnJvbSAyNjVjNmE0MWI3ODU5OTMwOWE4NjQ5ZDYxODc5ZjY5YjRhYWUwOTFkIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiAiSC5KLiBMdSIgPGhqbC50b29sc0BnbWFpbC5jb20+CkRhdGU6 IFdlZCwgMjIgSnVsIDIwMjAgMDc6MzM6NTQgLTA3MDAKU3ViamVjdDogW1BBVENIXSBMaW1pdCBw cjk1MjM3LTYuYyB0byB4ODYgdGFyZ2V0cwoKU2luY2UgYy1jKystY29tbW9uL3ByOTUyMzctNi5j IGlzIHg4NiBzcGVjaWZpYywgbGltaXQgaXQgdG8geDg2IHRhcmdldHMuCgoJUFIgdGFyZ2V0Lzk1 MjM3CgkqIGMtYysrLWNvbW1vbi9wcjk1MjM3LTYuYzogT25seSBydW4gZm9yIHg4NiB0YXJnZXRz LgotLS0KIGdjYy90ZXN0c3VpdGUvYy1jKystY29tbW9uL3ByOTUyMzctNi5jIHwgNCArKy0tCiAx IGZpbGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdp dCBhL2djYy90ZXN0c3VpdGUvYy1jKystY29tbW9uL3ByOTUyMzctNi5jIGIvZ2NjL3Rlc3RzdWl0 ZS9jLWMrKy1jb21tb24vcHI5NTIzNy02LmMKaW5kZXggY2UxNTY4ZmEyODIuLjVlNzM5NmQ3ZjRl IDEwMDY0NAotLS0gYS9nY2MvdGVzdHN1aXRlL2MtYysrLWNvbW1vbi9wcjk1MjM3LTYuYworKysg Yi9nY2MvdGVzdHN1aXRlL2MtYysrLWNvbW1vbi9wcjk1MjM3LTYuYwpAQCAtMSw1ICsxLDUgQEAK LS8qIHsgZGctZG8gcnVuIH0gKi8KLS8qIHsgZGctb3B0aW9ucyAiLU8yIiB7IHRhcmdldCB7IGk/ ODYtKi0qIHg4Nl82NC0qLSogfSB9IH0gKi8KKy8qIHsgZGctZG8gcnVuIHsgdGFyZ2V0IHsgaT84 Ni0qLSogeDg2XzY0LSotKiB9IH0gfSAqLworLyogeyBkZy1vcHRpb25zICItTzIiIH0gKi8KICNp bmNsdWRlIDxzdGRkZWYuaD4KICNpZmRlZiAgX194ODZfNjRfXwogIyBkZWZpbmUgRVhQX0FMSUdO IDgKLS0gCjIuMjYuMgoK --000000000000420da405ab08b160--