| Message ID | 20210407085610.sq4cd3kasglf2l2i@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Don't try and vmov labels [PR99647] | expand |
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
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.
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 }; +}