| Message ID | h48tv89hy5u.fsf_-_@genoa.aus.stglabs.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] rs6000: -flto forgets 'no-vsx' function attributes (PR target/70010) | expand |
Hi! On Wed, Oct 16, 2019 at 04:50:21PM +0800, Jiufu Guo wrote: > In PR70010, a function is marked with target(no-vsx) to disable VSX code > generation. To avoid VSX code generation, this function should not be > inlined into VSX function. > > In previous implementation, target of non-vsx is treated as subset target > with vsx, even user set no-vsx attribute. > > As discussed in previous mails, to fix the bug, in the current logic when > checking whether the caller's ISA flags supports the callee's ISA flags, we > just need to add a test that enforces that the caller's ISA flags match > exactly the callee's flags, for those flags that were explicitly set in the > callee. If caller without target attribute then using options from command > line. Here is a patch which is updated with comments from previous mails. > gcc/ > 2019-10-12 Peter Bergner <bergner@linux.ibm.com> > Jiufu Guo <guojiufu@linux.ibm.com> > > PR target/70010 > * config/rs6000/rs6000.c (rs6000_can_inline_p): Prohibit inlining if > the callee explicitly disables some isa_flags the caller is using. No trailing spaces please. > + /* If the caller has option attributes, then use them. > + Otherwise, use the command line options. */ > + if (caller_tree) > + caller_isa = TREE_TARGET_OPTION (caller_tree)->x_rs6000_isa_flags; > + else > + caller_isa = rs6000_isa_flags; Okay, it's very clear with that comment -- or it *seems* easy, anyway :-) > + /* The callee's options must be a subset of the caller's options, i.e. > + a vsx function may inline an altivec function, but a non-vsx function > + must not inline a vsx function. However, for those options that the no-vsx instead of non-vsx? Or make it clear even more explicitly that this is about having something explicitly disabled? (The code is clear though). > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-1.c > @@ -0,0 +1,17 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -flto -mvsx" } */ > + > +vector int c, a, b; > + > +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) > +foo () /* { dg-error "inlining failed in call to .* target specific option mismatch" } */ .* can match across lines. Not a huge deal here of course -- but maybe adding (?n) to the regexp works? (Just at the very start of it). > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-2.c > diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-3.c b/gcc/testsuite/gcc.target/powerpc/pr70010-3.c > new file mode 100644 Do you want to test anything in those two new testcases? Other than "it compiles"? > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-4.c > +foo () /* { dg-error "inlining failed in call to .* target specific option mismatch" } */ (.* again) > diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010.c b/gcc/testsuite/gcc.target/powerpc/pr70010.c > new file mode 100644 > index 0000000..affd0c5 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr70010.c > @@ -0,0 +1,20 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target powerpc_vsx_ok } */ This line isn't necessary anymore I think? Or if it is, it is needed in all these new testcases. > +/* { dg-options "-O2 -finline-functions" } */ > +/* { dg-final { scan-assembler {\mbl vadd_no_vsx\M} } } */ Okay for trunk. Thanks to both of you! Also okay for 9 and 8, after waiting a week to see if there is fallout. Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > Hi! > >> the callee explicitly disables some isa_flags the caller is using. > > No trailing spaces please. Updated. > >> + /* The callee's options must be a subset of the caller's options, i.e. >> + a vsx function may inline an altivec function, but a non-vsx function >> + must not inline a vsx function. However, for those options that the > > no-vsx instead of non-vsx? Or make it clear even more explicitly that this is > about having something explicitly disabled? (The code is clear > though). Thanks. > >> + >> +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) >> +foo () /* { dg-error "inlining failed in call to .* target specific option mismatch" } */ > > .* can match across lines. Not a huge deal here of course -- but maybe > adding (?n) to the regexp works? (Just at the very start of it). Seems (?n) does not help this case, so keep old one. > >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-2.c > >> diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-3.c b/gcc/testsuite/gcc.target/powerpc/pr70010-3.c >> new file mode 100644 > > Do you want to test anything in those two new testcases? Other than "it > compiles"? If compiling pass, it indicates inlining works fine for the caller and callee. So, I did not check other thing here. > >> +/* { dg-require-effective-target powerpc_vsx_ok } */ > > This line isn't necessary anymore I think? Or if it is, it is needed in > all these new testcases. Updated. > > Okay for trunk. Thanks to both of you! > > Also okay for 9 and 8, after waiting a week to see if there is fallout. > > > Segher I just committed the patch. Thanks all for your helps! Segher, Richard, Peter, Andreas, Iain and all. Jiufu BR
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index d1434a9..26985d3 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -23964,25 +23964,31 @@ rs6000_can_inline_p (tree caller, tree callee) tree caller_tree = DECL_FUNCTION_SPECIFIC_TARGET (caller); tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (callee); - /* If callee has no option attributes, then it is ok to inline. */ + /* If the callee has no option attributes, then it is ok to inline. */ if (!callee_tree) ret = true; - /* If caller has no option attributes, but callee does then it is not ok to - inline. */ - else if (!caller_tree) - ret = false; - else { - struct cl_target_option *caller_opts = TREE_TARGET_OPTION (caller_tree); + HOST_WIDE_INT caller_isa; struct cl_target_option *callee_opts = TREE_TARGET_OPTION (callee_tree); + HOST_WIDE_INT callee_isa = callee_opts->x_rs6000_isa_flags; + HOST_WIDE_INT explicit_isa = callee_opts->x_rs6000_isa_flags_explicit; - /* Callee's options should a subset of the caller's, i.e. a vsx function - can inline an altivec function but a non-vsx function can't inline a - vsx function. */ - if ((caller_opts->x_rs6000_isa_flags & callee_opts->x_rs6000_isa_flags) - == callee_opts->x_rs6000_isa_flags) + /* If the caller has option attributes, then use them. + Otherwise, use the command line options. */ + if (caller_tree) + caller_isa = TREE_TARGET_OPTION (caller_tree)->x_rs6000_isa_flags; + else + caller_isa = rs6000_isa_flags; + + /* The callee's options must be a subset of the caller's options, i.e. + a vsx function may inline an altivec function, but a non-vsx function + must not inline a vsx function. However, for those options that the + callee has explicitly set, then we must enforce that the callee's + and caller's options match exactly; see PR70010. */ + if (((caller_isa & callee_isa) == callee_isa) + && (caller_isa & explicit_isa) == (callee_isa & explicit_isa)) ret = true; } diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-1.c b/gcc/testsuite/gcc.target/powerpc/pr70010-1.c new file mode 100644 index 0000000..78870db --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-1.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -flto -mvsx" } */ + +vector int c, a, b; + +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) +foo () /* { dg-error "inlining failed in call to .* target specific option mismatch" } */ +{ + c = a + b; +} + +int +main () +{ + foo (); /* { dg-message "called from here" } */ + c = a + b; +} diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-2.c b/gcc/testsuite/gcc.target/powerpc/pr70010-2.c new file mode 100644 index 0000000..4c09b21 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-2.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -flto -mno-vsx" } */ + +vector int c, a, b; + +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) +foo () +{ + c = a + b; +} + +int +main () +{ + foo (); + c = a + b; +} diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-3.c b/gcc/testsuite/gcc.target/powerpc/pr70010-3.c new file mode 100644 index 0000000..bca3187 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-3.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -mno-vsx" } */ + +vector int c, a, b; + +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) +foo () +{ + c = a + b; +} + +int +main () +{ + foo (); + c = a + b; +} diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010-4.c b/gcc/testsuite/gcc.target/powerpc/pr70010-4.c new file mode 100644 index 0000000..c575cff --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr70010-4.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -mvsx" } */ + +vector int c, a, b; + +static inline void __attribute__ ((__always_inline__, target ("no-vsx"))) +foo () /* { dg-error "inlining failed in call to .* target specific option mismatch" } */ +{ + c = a + b; +} + +int +main () +{ + foo (); /* { dg-message "called from here" } */ + c = a + b; +} diff --git a/gcc/testsuite/gcc.target/powerpc/pr70010.c b/gcc/testsuite/gcc.target/powerpc/pr70010.c new file mode 100644 index 0000000..affd0c5 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr70010.c @@ -0,0 +1,20 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-options "-O2 -finline-functions" } */ +/* { dg-final { scan-assembler {\mbl vadd_no_vsx\M} } } */ + +typedef int vec_t __attribute__((vector_size(16))); + +static vec_t +__attribute__((__target__("no-vsx"))) +vadd_no_vsx (vec_t a, vec_t b) +{ + return a + b; +} + +vec_t +__attribute__((__target__("vsx"))) +call_vadd_no_vsx (vec_t x, vec_t y, vec_t z) +{ + return vadd_no_vsx (x, y) - z; +}