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 <berg...@linux.ibm.com>
> Jiufu Guo <guoji...@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
