| Message ID | 5644AB37.3050201@arm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira <Andre.SimoesDiasVieira@arm.com> wrote: > Hi, > > This patch changes this testcase to make sure LTO will not optimize away > the assignment of the local array to a global variable which was introduced > to make sure stack space was made available for the test to work. > > This is correct because LTO is supposed to optimize this global away as at > link time it knows this global will never be read. By adding a read of the > global, LTO will no longer optimize it away. But that's only because we can't see that bar doesn't clobber it, else we would optimize away the check and get here again. Much better to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely. Richard. > Tested by running regressions for this testcase for various ARM targets. > > Is this OK to commit? > > Thanks, > Andre Vieira > > gcc/testsuite/ChangeLog: > 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> > > * gcc.dg/torture/stackalign/builtin-return-1.c: Added read > to global such that a write is not optimized away by LTO.
On 13/11/15 10:34, Richard Biener wrote: > On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira > <Andre.SimoesDiasVieira@arm.com> wrote: >> Hi, >> >> This patch changes this testcase to make sure LTO will not optimize away >> the assignment of the local array to a global variable which was introduced >> to make sure stack space was made available for the test to work. >> >> This is correct because LTO is supposed to optimize this global away as at >> link time it knows this global will never be read. By adding a read of the >> global, LTO will no longer optimize it away. > > But that's only because we can't see that bar doesn't clobber it, else > we would optimize away the check and get here again. Much better > to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely. > > Richard. > >> Tested by running regressions for this testcase for various ARM targets. >> >> Is this OK to commit? >> >> Thanks, >> Andre Vieira >> >> gcc/testsuite/ChangeLog: >> 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> >> >> * gcc.dg/torture/stackalign/builtin-return-1.c: Added read >> to global such that a write is not optimized away by LTO. > Hi Richard, That would be great but __attribute__ ((used)) can only be used for static variables and making dummy static would defeat the purpose here. Cheers, Andre
On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira <Andre.SimoesDiasVieira@arm.com> wrote: > On 13/11/15 10:34, Richard Biener wrote: >> >> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira >> <Andre.SimoesDiasVieira@arm.com> wrote: >>> >>> Hi, >>> >>> This patch changes this testcase to make sure LTO will not optimize >>> away >>> the assignment of the local array to a global variable which was >>> introduced >>> to make sure stack space was made available for the test to work. >>> >>> This is correct because LTO is supposed to optimize this global away >>> as at >>> link time it knows this global will never be read. By adding a read of >>> the >>> global, LTO will no longer optimize it away. >> >> >> But that's only because we can't see that bar doesn't clobber it, else >> we would optimize away the check and get here again. Much better >> to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely. >> >> Richard. >> >>> Tested by running regressions for this testcase for various ARM >>> targets. >>> >>> Is this OK to commit? >>> >>> Thanks, >>> Andre Vieira >>> >>> gcc/testsuite/ChangeLog: >>> 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> >>> >>> * gcc.dg/torture/stackalign/builtin-return-1.c: Added read >>> to global such that a write is not optimized away by LTO. >> >> > Hi Richard, > > That would be great but __attribute__ ((used)) can only be used for static > variables and making dummy static would defeat the purpose here. I see. What about volatile? > Cheers, > Andre >
On 16/11/15 13:33, Richard Biener wrote: > On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira > <Andre.SimoesDiasVieira@arm.com> wrote: >> On 13/11/15 10:34, Richard Biener wrote: >>> >>> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira >>> <Andre.SimoesDiasVieira@arm.com> wrote: >>>> >>>> Hi, >>>> >>>> This patch changes this testcase to make sure LTO will not optimize >>>> away >>>> the assignment of the local array to a global variable which was >>>> introduced >>>> to make sure stack space was made available for the test to work. >>>> >>>> This is correct because LTO is supposed to optimize this global away >>>> as at >>>> link time it knows this global will never be read. By adding a read of >>>> the >>>> global, LTO will no longer optimize it away. >>> >>> >>> But that's only because we can't see that bar doesn't clobber it, else >>> we would optimize away the check and get here again. Much better >>> to mark 'dummy' with __attribute__((used)) and go away with 'g' entirely. >>> >>> Richard. >>> >>>> Tested by running regressions for this testcase for various ARM >>>> targets. >>>> >>>> Is this OK to commit? >>>> >>>> Thanks, >>>> Andre Vieira >>>> >>>> gcc/testsuite/ChangeLog: >>>> 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> >>>> >>>> * gcc.dg/torture/stackalign/builtin-return-1.c: Added read >>>> to global such that a write is not optimized away by LTO. >>> >>> >> Hi Richard, >> >> That would be great but __attribute__ ((used)) can only be used for static >> variables and making dummy static would defeat the purpose here. > > I see. What about volatile? > >> Cheers, >> Andre >> > Yeh a 'volatile char dummy[64];' with a read afterwards leads to the stack still being reserved after LTO and we won't need the global variable. If you prefer that solution Ill respin the patch. Cheers, Andre
On Mon, Nov 16, 2015 at 3:08 PM, Andre Vieira <Andre.SimoesDiasVieira@arm.com> wrote: > On 16/11/15 13:33, Richard Biener wrote: >> >> On Mon, Nov 16, 2015 at 12:43 PM, Andre Vieira >> <Andre.SimoesDiasVieira@arm.com> wrote: >>> >>> On 13/11/15 10:34, Richard Biener wrote: >>>> >>>> >>>> On Thu, Nov 12, 2015 at 4:07 PM, Andre Vieira >>>> <Andre.SimoesDiasVieira@arm.com> wrote: >>>>> >>>>> >>>>> Hi, >>>>> >>>>> This patch changes this testcase to make sure LTO will not optimize >>>>> away >>>>> the assignment of the local array to a global variable which was >>>>> introduced >>>>> to make sure stack space was made available for the test to work. >>>>> >>>>> This is correct because LTO is supposed to optimize this global >>>>> away >>>>> as at >>>>> link time it knows this global will never be read. By adding a read of >>>>> the >>>>> global, LTO will no longer optimize it away. >>>> >>>> >>>> >>>> But that's only because we can't see that bar doesn't clobber it, else >>>> we would optimize away the check and get here again. Much better >>>> to mark 'dummy' with __attribute__((used)) and go away with 'g' >>>> entirely. >>>> >>>> Richard. >>>> >>>>> Tested by running regressions for this testcase for various ARM >>>>> targets. >>>>> >>>>> Is this OK to commit? >>>>> >>>>> Thanks, >>>>> Andre Vieira >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> >>>>> >>>>> * gcc.dg/torture/stackalign/builtin-return-1.c: Added read >>>>> to global such that a write is not optimized away by LTO. >>>> >>>> >>>> >>> Hi Richard, >>> >>> That would be great but __attribute__ ((used)) can only be used for >>> static >>> variables and making dummy static would defeat the purpose here. >> >> >> I see. What about volatile? >> >>> Cheers, >>> Andre >>> >> > Yeh a 'volatile char dummy[64];' with a read afterwards leads to the stack > still being reserved after LTO and we won't need the global variable. > > If you prefer that solution Ill respin the patch. Yes please. Richard. > Cheers, > Andre >
From 6fbac447475c3b669baee84aa9d6291c3d09f1ab Mon Sep 17 00:00:00 2001 From: Andre Simoes Dias Vieira <andsim01@arm.com> Date: Fri, 6 Nov 2015 13:13:47 +0000 Subject: [PATCH] keep the stack testsuite fix --- gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c index af017532aeb3878ef7ad717a2743661a87a56b7d..1ccd109256de72419a3c71c2c1be6d07c423c005 100644 --- a/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c +++ b/gcc/testsuite/gcc.dg/torture/stackalign/builtin-return-1.c @@ -39,5 +39,10 @@ int main(void) if (bar(1) != 2) abort(); + /* Make sure there is a read of the global after the function call to bar + * such that LTO does not optimize away the assignment above. */ + if (g != dummy) + abort(); + return 0; } -- 1.9.1
Hi, This patch changes this testcase to make sure LTO will not optimize away the assignment of the local array to a global variable which was introduced to make sure stack space was made available for the test to work. This is correct because LTO is supposed to optimize this global away as at link time it knows this global will never be read. By adding a read of the global, LTO will no longer optimize it away. Tested by running regressions for this testcase for various ARM targets. Is this OK to commit? Thanks, Andre Vieira gcc/testsuite/ChangeLog: 2015-11-06 Andre Vieira <andre.simoesdiasvieira@arm.com> * gcc.dg/torture/stackalign/builtin-return-1.c: Added read to global such that a write is not optimized away by LTO.