[V2] rs6000: -flto forgets 'no-vsx' function attributes (PR target/70010)

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)
Related show

Commit Message

Jiufu Guo Oct. 16, 2019, 8:50 a.m.
Hi,

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.  

Bootstrapped/regtested on ppc64le-linux, ok for trunk, and backport to 9?

Jiufu
BR


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.  

gcc.testsuite/
2019-10-12  Peter Bergner <bergner@linux.ibm.com>
	    Jiufu Guo  <guojiufu@linux.ibm.com>

	PR target/70010
	* gcc.target/powerpc/pr70010.c: New test.  
	* gcc.target/powerpc/pr70010-1.c: New test.  
	* gcc.target/powerpc/pr70010-2.c: New test.  
	* gcc.target/powerpc/pr70010-3.c: New test.  
	* gcc.target/powerpc/pr70010-4.c: New test.

Comments

Segher Boessenkool Oct. 16, 2019, 9:47 a.m. | #1
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
Jiufu Guo Oct. 16, 2019, 1:49 p.m. | #2
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

Patch

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;
+}