| Message ID | 20170118102150.GJ1867@tucnak |
|---|---|
| State | New |
| Headers | show |
On 01/18/2017 11:21 AM, Jakub Jelinek wrote: > So my preference is to add -fno-ipa-icf to dg-options and > add the missing dg-error line on line 42. > > Like below, ok for trunk? I like the idea of adding -fno-ipa-icf to the test-case. Martin
On 01/18/2017 11:21 AM, Jakub Jelinek wrote: > 2017-01-18 Jakub Jelinek <jakub@redhat.com> > > * gcc.target/s390/target-attribute/tattr-2.c: Add -fno-ipa-icf > to dg-options. > (p0): Add missing dg-error. Ok. Thanks! -Andreas-
On Wed, Jan 18, 2017 at 11:21:50AM +0100, Jakub Jelinek wrote: > On Wed, Jan 18, 2017 at 10:53:59AM +0100, Martin Liška wrote: > > As I've been reading when the warning is emitted (rtl expansion), I guess a pair of function was merged > > by IPA ICF and thus the location points to a different function. > > > > I can't reproduce the test-case due to missing __HTM__. Can you please provide pre-processed source file? > > The testcase doesn't include any headers, just compile it with a cross to > s390x-linux. That said, IMHO the test should either use -fno-ipa-icf, > or use something to avoid ICFing of functions it doesn't want to fold > (say use int return type instead of void and return a different constant in > each case). > > I think the testcase as written just assumes ICF happens but in a certain > way and what broke is that the same functions are ICFed in a different order > now, because with -fno-ipa-icf it reports: > tattr-2.c: In function ‘p0’: > tattr-2.c:23:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > __builtin_tend (); > ^~~~~~~~~~~~~~~~~ > tattr-2.c: In function ‘a0’: > tattr-2.c:42:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ > ^~~~~~~~~~~~~~~~~ > tattr-2.c: In function ‘htmd’: > tattr-2.c:50:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ > ^~~~~~~~~~~~~~~~~ > So my preference is to add -fno-ipa-icf to dg-options and > add the missing dg-error line on line 42. You mean line 23. Does -fno-ipa-icf also enable the error there? > Like below, ok for trunk? Looks good. > > 2017-01-18 Jakub Jelinek <jakub@redhat.com> > > * gcc.target/s390/target-attribute/tattr-2.c: Add -fno-ipa-icf > to dg-options. > (p0): Add missing dg-error. > > --- gcc/testsuite/gcc.target/s390/target-attribute/tattr-2.c.jj 2015-12-04 17:19:09.000000000 +0100 > +++ gcc/testsuite/gcc.target/s390/target-attribute/tattr-2.c 2017-01-18 11:18:15.674196392 +0100 > @@ -2,7 +2,7 @@ > > /* { dg-do compile */ > /* { dg-require-effective-target target_attribute } */ > -/* { dg-options "-O3 -march=zEC12 -mno-htm" } */ > +/* { dg-options "-O3 -march=zEC12 -mno-htm -fno-ipa-icf" } */ > > #pragma GCC target("htm") > void p1(void) > @@ -20,7 +20,7 @@ void p0(void) > #ifdef __HTM__ > #error __HTM__ is defined > #endif > - __builtin_tend (); > + __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ > } > #pragma GCC reset_options Ciao Dominik ^_^ ^_^
Hi! On Wed, Jan 18, 2017 at 12:49:19PM +0100, Dominik Vogt wrote: > > I think the testcase as written just assumes ICF happens but in a certain > > way and what broke is that the same functions are ICFed in a different order > > now, because with -fno-ipa-icf it reports: > > tattr-2.c: In function ‘p0’: > > tattr-2.c:23:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > > __builtin_tend (); > > ^~~~~~~~~~~~~~~~~ > > tattr-2.c: In function ‘a0’: > > tattr-2.c:42:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > > __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ > > ^~~~~~~~~~~~~~~~~ > > tattr-2.c: In function ‘htmd’: > > tattr-2.c:50:3: error: Builtin ‘__builtin_tend’ is not supported without -mhtm (default with -march=zEC12 and higher). > > __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ > > ^~~~~~~~~~~~~~~~~ > > So my preference is to add -fno-ipa-icf to dg-options and > > add the missing dg-error line on line 42. > > You mean line 23. Does -fno-ipa-icf also enable the error there? > > > Like below, ok for trunk? > > Looks good. Committed to trunk. Two more things: 1) 823 error ("Builtin %qF is not supported without -mhtm " 824 "(default with -march=zEC12 and higher).", fndecl); is a bug, diagnostics (I think Fortran FE is a big exception here) should not start with capital letters, so it should be "builtin instead of "Builtin . Please look at other diagnostics in config/s390 2) what happens on the testcase is actually quite nasty, which is why you get 3 errors instead of just 2 I was expecting with the default -fipa-icf - first ICF optimizes a0 + p0 and a1 + p1 calls, with current trunk a0 and a1 are thunks that call p0 or p1. And then comes inlining and inlines p0 into a0 and p1 into a1, which basically undoes the ICF optimization, but not even that, it is worse than before because the debug info as well as locations are as if p0 has been an inline function inlined into a0. Dunno if ICF shouldn't check if the functions aren't small enough not to be worth ICFing them together, or if inlining shouldn't recognize ICF created thunks and not inline into them Jakub
On 01/18/2017 01:05 PM, Jakub Jelinek wrote: ... > Two more things: > 1) > 823 error ("Builtin %qF is not supported without -mhtm " > 824 "(default with -march=zEC12 and higher).", fndecl); > is a bug, diagnostics (I think Fortran FE is a big exception here) should > not start with capital letters, so it should be "builtin instead of > "Builtin . Please look at other diagnostics in config/s390 These have been added by me. I'll commit a patch. -Andreas-
--- gcc/testsuite/gcc.target/s390/target-attribute/tattr-2.c.jj 2015-12-04 17:19:09.000000000 +0100 +++ gcc/testsuite/gcc.target/s390/target-attribute/tattr-2.c 2017-01-18 11:18:15.674196392 +0100 @@ -2,7 +2,7 @@ /* { dg-do compile */ /* { dg-require-effective-target target_attribute } */ -/* { dg-options "-O3 -march=zEC12 -mno-htm" } */ +/* { dg-options "-O3 -march=zEC12 -mno-htm -fno-ipa-icf" } */ #pragma GCC target("htm") void p1(void) @@ -20,7 +20,7 @@ void p0(void) #ifdef __HTM__ #error __HTM__ is defined #endif - __builtin_tend (); + __builtin_tend (); /* { dg-error "is not supported without -mhtm" } */ } #pragma GCC reset_options