From: Richard Biener <richard.guenther@gmail.com>
To: Sunil Pandey <skpgkp1@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
"H.J. Lu" <hjl.tools@gmail.com>,
Jason Merrill <jason@redhat.com>,
"Joseph S. Myers" <joseph@codesourcery.com>,
Sunil K Pandey <skpgkp2@gmail.com>
Subject: Re: [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237]
Date: Fri, 17 Jul 2020 10:22:42 +0200 [thread overview]
Message-ID: <CAFiYyc0AhWBNGRGYacpZVjVu5e_EpQ-_Vf4S9nLU_J-VtCHWmg@mail.gmail.com> (raw)
In-Reply-To: <CAFMdu1+NAUWm0hQye=_Cuqf5TYV+tbN4f7kQKZqiS+uM2BUBng@mail.gmail.com>
On Fri, Jul 17, 2020 at 7:15 AM Sunil Pandey <skpgkp1@gmail.com> wrote:
>
> Any comment on revised patch? At least, in finish_decl, decl global attributes are populated.
+static void
+ix86_lower_local_decl_alignment (tree decl)
+{
+ unsigned new_align = LOCAL_DECL_ALIGNMENT (decl);
please use the macro-expanded call here since we want to amend
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 this
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-overs (your identified
inliner case) as missed optimization [for the linux kernel which appearantly
decided that -mpreferred-stack-boundary=2 is a good ABI to use].
Richard.
> On Tue, Jul 14, 2020 at 8:37 AM Sunil Pandey <skpgkp1@gmail.com> wrote:
>>
>> On Sat, Jul 4, 2020 at 9:11 AM Richard Biener
>> <richard.guenther@gmail.com> wrote:
>> >
>> > On July 3, 2020 11:16:46 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
>> > >On 6/29/20 5:00 AM, Richard Biener wrote:
>> > >> On Fri, Jun 26, 2020 at 10:11 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>> > >>>
>> > >>> On Thu, Jun 25, 2020 at 1:10 AM Richard Biener
>> > >>> <richard.guenther@gmail.com> wrote:
>> > >>>>
>> > >>>> On Thu, Jun 25, 2020 at 2:53 AM Sunil Pandey <skpgkp1@gmail.com>
>> > >wrote:
>> > >>>>>
>> > >>>>> On Wed, Jun 24, 2020 at 12:30 AM Richard Biener
>> > >>>>> <richard.guenther@gmail.com> wrote:
>> > >>>>>>
>> > >>>>>> On Tue, Jun 23, 2020 at 5:31 PM Sunil K Pandey via Gcc-patches
>> > >>>>>> <gcc-patches@gcc.gnu.org> wrote:
>> > >>>>>>>
>> > >>>>>>> From: Sunil K Pandey <skpgkp1@gmail.com>
>> > >>>>>>>
>> > >>>>>>> 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)
>> > >>>>> <var_decl 0x7ffff7ffbb40 x
>> > >>>>> type <integer_type 0x7fffea801888 long long int DI
>> > >>>>> size <integer_cst 0x7fffea7e8d38 constant 64>
>> > >>>>> unit-size <integer_cst 0x7fffea7e8d50 constant 8>
>> > >>>>> align:64 warn_if_not_align:0 symtab:0 alias-set -1
>> > >>>>> canonical-type 0x7fffea801888 precision:64 min <integer_cst
>> > >>>>> 0x7fffea7e8fd8 -9223372036854775808> max <integer_cst
>> > >0x7fffea806000
>> > >>>>> 9223372036854775807>
>> > >>>>> pointer_to_this <pointer_type 0x7fffea8110a8>>
>> > >>>>> DI foo.c:1:11 size <integer_cst 0x7fffea7e8d38 64> unit-size
>> > >>>>> <integer_cst 0x7fffea7e8d50 8>
>> > >>>>> 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
>> >
next prev parent reply other threads:[~2020-07-17 8:22 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-23 15:29 [PATCH] Add TARGET_UPDATE_DECL_ALIGNMENT [PR95237] Sunil K Pandey
2020-06-24 7:30 ` Richard Biener
2020-06-25 0:52 ` Sunil Pandey
2020-06-25 8:10 ` Richard Biener
2020-06-26 20:11 ` [PATCH] Add TARGET_LOWER_LOCAL_DECL_ALIGNMENT [PR95237] H.J. Lu
2020-06-29 9:00 ` Richard Biener
2020-07-03 21:16 ` Jason Merrill
2020-07-04 16:11 ` Richard Biener
2020-07-14 15:37 ` Sunil Pandey
2020-07-17 5:15 ` Sunil Pandey
2020-07-17 8:22 ` Richard Biener [this message]
2020-07-18 5:57 ` Sunil Pandey
2020-07-20 12:06 ` Richard Biener
2020-07-21 5:16 ` Sunil Pandey
2020-07-21 7:50 ` Richard Biener
2020-07-21 23:04 ` Sunil Pandey
2020-07-22 14:24 ` Dimitar Dimitrov
2020-07-22 14:37 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc0AhWBNGRGYacpZVjVu5e_EpQ-_Vf4S9nLU_J-VtCHWmg@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
--cc=jason@redhat.com \
--cc=joseph@codesourcery.com \
--cc=skpgkp1@gmail.com \
--cc=skpgkp2@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).