arm: Don't try and vmov labels [PR99647]

Message ID 20210407085610.sq4cd3kasglf2l2i@arm.com
State New
Headers show
Series
  • arm: Don't try and vmov labels [PR99647]
Related show

Commit Message

apinski--- via Gcc-patches April 7, 2021, 8:56 a.m.
Hi all,

When investigating this PR, I was initially confused as to why we were
matching a vec_duplicate on the RHS of *mve_mov<mode> (since
general_operand does not match vec_duplicates). It turns out that there
are two patterns in mve.md named *mve_mov<mode> and the second one
matches vec_duplicates. I've renamed this pattern to *mve_vdup<mode> to
avoid further confusion.

The issue in the PR is that the predicate for the operand of the
vec_duplicate in *mve_vdup<mode> is not strict enough: it allows (e.g.)
labels when we really only want to allow registers and const_ints.

Testing:
 * Bootstrapped and regtested on arm-linux-gnueabihf, no regressions.
 * Regtested an arm-eabi cross configured with --with-float=hard
 --with-arch=armv8.1-m.main+mve: no regressions. Also fixes a couple of
 existing torture test failures at -O3 for gcc.dg/torture/pr47958-1.c.

OK for trunk and eventual backport to the GCC 10 branch?

Thanks,
Alex

gcc/ChangeLog:

	PR target/99647
	* config/arm/mve.md (*mve_mov<mode>): Rename to...
	(*mve_vdup<mode>): ... this. Also tighten up predicate for
	vec_duplicate operand.

gcc/testsuite/ChangeLog:

	PR target/99647
	* gcc.c-torture/compile/pr99647.c: New test.

Comments

apinski--- via Gcc-patches April 7, 2021, 9:38 a.m. | #1
Alex Coplan via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Hi all,

>

> When investigating this PR, I was initially confused as to why we were

> matching a vec_duplicate on the RHS of *mve_mov<mode> (since

> general_operand does not match vec_duplicates). It turns out that there

> are two patterns in mve.md named *mve_mov<mode> and the second one

> matches vec_duplicates. I've renamed this pattern to *mve_vdup<mode> to

> avoid further confusion.

>

> The issue in the PR is that the predicate for the operand of the

> vec_duplicate in *mve_vdup<mode> is not strict enough: it allows (e.g.)

> labels when we really only want to allow registers and const_ints.

>

> Testing:

>  * Bootstrapped and regtested on arm-linux-gnueabihf, no regressions.

>  * Regtested an arm-eabi cross configured with --with-float=hard

>  --with-arch=armv8.1-m.main+mve: no regressions. Also fixes a couple of

>  existing torture test failures at -O3 for gcc.dg/torture/pr47958-1.c.

>

> OK for trunk and eventual backport to the GCC 10 branch?

>

> Thanks,

> Alex

>

> gcc/ChangeLog:

>

> 	PR target/99647

> 	* config/arm/mve.md (*mve_mov<mode>): Rename to...

> 	(*mve_vdup<mode>): ... this. Also tighten up predicate for

> 	vec_duplicate operand.

>

> gcc/testsuite/ChangeLog:

>

> 	PR target/99647

> 	* gcc.c-torture/compile/pr99647.c: New test.

>

> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md

> index 135186371e7..d79b3156077 100644

> --- a/gcc/config/arm/mve.md

> +++ b/gcc/config/arm/mve.md

> @@ -104,10 +104,10 @@ (define_insn "*mve_mov<mode>"

>     (set_attr "thumb2_pool_range" "*,*,*,*,1018,*,*,*,*")

>     (set_attr "neg_pool_range" "*,*,*,*,996,*,*,*,*")])

>  

> -(define_insn "*mve_mov<mode>"

> +(define_insn "*mve_vdup<mode>"

>    [(set (match_operand:MVE_types 0 "s_register_operand" "=w,w")

>  	(vec_duplicate:MVE_types

> -	  (match_operand:SI 1 "nonmemory_operand" "r,i")))]

> +	  (match_operand:SI 1 "reg_or_int_operand" "r,i")))]


When an operand accepts registers, having a predicate that's too lenient
can be bad for optimisation (and so is worth fixing for that reason),
but it shouldn't affect correctness.  I think the bug here is really
with the "i" constraint.  When constant and non-constant alternatives
are combined like this, the constraints have to be precise about which
constants they accept.  (When all alternatives are constant, there is
nothing to reload, and so checking in the predicate is enough.)

As it stands, if the RA sees:

  (set (reg:SI R1) (symbol_ref:SI …))    ; only assignment to R1
  ...
  (set (reg:V4SI R2) (vec_duplicate:V4SI (reg:SI R1)))

and can't allocate a register to R1, it can try to rematerialise R1
directly inside the vec_duplicate:

  (set (reg:V4SI R2) (vec_duplicate:V4SI (symbol_ref:SI …)))

This will match the "i" constraint and so appear to be OK.  We'll probably
later get an ICE because the symbol_ref doesn't match the predicate
(which is something that the RA isn't required to check).

In practice, the MVE instruction doesn't accept all const_ints either,
so there needs to be a range check of some kind.

However, (vec_duplicate (const_int …)) isn't canonical RTL: it should be
a (const_vector …) instead.  Those const_vectors should already be going
through the “real” move patterns.

I think the easiest fix would therefore be to remove the second alternative
and make this a register-only pattern.

There's a later:

(define_insn "*mve_vec_duplicate<mode>"

which I think would be worth removing as well, since it provides a subset
of the same functionality.  However, that pattern is correct in using
<V_elem> as the mode of the element, whereas above we use SI for all modes.

Thanks,
Richard
apinski--- via Gcc-patches April 7, 2021, 9:51 a.m. | #2
Hi Alex,

> -----Original Message-----

> From: Alex Coplan <Alex.Coplan@arm.com>

> Sent: 07 April 2021 09:56

> To: gcc-patches@gcc.gnu.org

> Cc: nickc@redhat.com; Richard Earnshaw <Richard.Earnshaw@arm.com>;

> Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>; Kyrylo

> Tkachov <Kyrylo.Tkachov@arm.com>

> Subject: [PATCH] arm: Don't try and vmov labels [PR99647]

> 

> Hi all,

> 

> When investigating this PR, I was initially confused as to why we were

> matching a vec_duplicate on the RHS of *mve_mov<mode> (since

> general_operand does not match vec_duplicates). It turns out that there

> are two patterns in mve.md named *mve_mov<mode> and the second one

> matches vec_duplicates. I've renamed this pattern to *mve_vdup<mode> to

> avoid further confusion.

> 

> The issue in the PR is that the predicate for the operand of the

> vec_duplicate in *mve_vdup<mode> is not strict enough: it allows (e.g.)

> labels when we really only want to allow registers and const_ints.

> 

> Testing:

>  * Bootstrapped and regtested on arm-linux-gnueabihf, no regressions.

>  * Regtested an arm-eabi cross configured with --with-float=hard

>  --with-arch=armv8.1-m.main+mve: no regressions. Also fixes a couple of

>  existing torture test failures at -O3 for gcc.dg/torture/pr47958-1.c.

> 

> OK for trunk and eventual backport to the GCC 10 branch?


Ok for trunk.
I think GCC 10.3 is due for release today or tomorrow, so it'll likely have to wait for GCC 10.4 backport.
Thanks,
Kyrill

> 

> Thanks,

> Alex

> 

> gcc/ChangeLog:

> 

> 	PR target/99647

> 	* config/arm/mve.md (*mve_mov<mode>): Rename to...

> 	(*mve_vdup<mode>): ... this. Also tighten up predicate for

> 	vec_duplicate operand.

> 

> gcc/testsuite/ChangeLog:

> 

> 	PR target/99647

> 	* gcc.c-torture/compile/pr99647.c: New test.

Patch

diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
index 135186371e7..d79b3156077 100644
--- a/gcc/config/arm/mve.md
+++ b/gcc/config/arm/mve.md
@@ -104,10 +104,10 @@  (define_insn "*mve_mov<mode>"
    (set_attr "thumb2_pool_range" "*,*,*,*,1018,*,*,*,*")
    (set_attr "neg_pool_range" "*,*,*,*,996,*,*,*,*")])
 
-(define_insn "*mve_mov<mode>"
+(define_insn "*mve_vdup<mode>"
   [(set (match_operand:MVE_types 0 "s_register_operand" "=w,w")
 	(vec_duplicate:MVE_types
-	  (match_operand:SI 1 "nonmemory_operand" "r,i")))]
+	  (match_operand:SI 1 "reg_or_int_operand" "r,i")))]
   "TARGET_HAVE_MVE || TARGET_HAVE_MVE_FLOAT"
 {
   if (which_alternative == 0)
diff --git a/gcc/testsuite/gcc.c-torture/compile/pr99647.c b/gcc/testsuite/gcc.c-torture/compile/pr99647.c
new file mode 100644
index 00000000000..701155dd856
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/pr99647.c
@@ -0,0 +1,5 @@ 
+/* { dg-do assemble } */
+typedef int __attribute((vector_size(16))) V;
+V f(void) {
+  return (V){ (int)f, (int)f, (int)f, (int)f };
+}