Re: [PATCH] RISC-V: Implement movmisalign<mode> to enable SLP

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

On Thu, 10 Nov 2022 at 02:24, Kito Cheng <kito.cheng@gmail.com> wrote:
>
> I am not sure if I am missing something, your testcase should rely on
> movmisalignhi pattern, but you defined movmisalign<mode> with ANYF
> mode iterator rather than movmisalign<mode> with HI, SI, DI?


It was already defined with the ANYI iterator in the patch, but that
seems to be a moot point...

>
> And seems the testcase compile with `-march=rv64gc -mabi=lp64
> -mtune=size -O2` w/o this patch already generated lhu/sh pair?


...as this change is needed on our GCC 12.x tree, but the current
trunk seems to correctly form the lhu on master (at least for the
artificial testcase) even without it.
Thanks for catching this!

I'll put this back to the end of the queue: this has to be looked at
with the original underlying issue in SPEC CPU 2017.
You'll probably not hear more on this specific case until after the
close of phase 1.

—Philipp.

>
> On Wed, Nov 9, 2022 at 3:08 PM Philipp Tomsich <philipp.tomsich@vrull.eu> wrote:
> >
> > The default implementation of support_vector_misalignment() checks
> > whether movmisalign<mode> is present for the requested mode.  This
> > will be used by vect_supportable_dr_alignment() to determine whether a
> > misaligned access of vectorized data is permissible.
> >
> > For RISC-V this is required to convert multiple integer data refs,
> > such as "c[1] << 8) | c[0]" into a larger (in the example before: a
> > halfword load) access.
> > We conditionalize on !riscv_slow_unaligned_access_p to allow the
> > misaligned refs, if they are not expected to be slow.
> >
> > This benefits both xalancbmk and blender on SPEC CPU 2017.
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/riscv.md (movmisalign<mode>): Implement.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/riscv/movmisalign-1.c: New test.
> >         * gcc.target/riscv/movmisalign-2.c: New test.
> >         * gcc.target/riscv/movmisalign-3.c: New test.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >  gcc/config/riscv/riscv.md                      | 18 ++++++++++++++++++
> >  gcc/testsuite/gcc.target/riscv/movmisalign-1.c | 12 ++++++++++++
> >  gcc/testsuite/gcc.target/riscv/movmisalign-2.c | 12 ++++++++++++
> >  gcc/testsuite/gcc.target/riscv/movmisalign-3.c | 12 ++++++++++++
> >  4 files changed, 54 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/movmisalign-1.c
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/movmisalign-2.c
> >  create mode 100644 gcc/testsuite/gcc.target/riscv/movmisalign-3.c
> >
> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> > index 289ff7470c6..1b357a9c57f 100644
> > --- a/gcc/config/riscv/riscv.md
> > +++ b/gcc/config/riscv/riscv.md
> > @@ -1715,6 +1715,24 @@
> >                       MAX_MACHINE_MODE, &operands[3], TRUE);
> >  })
> >
> > +;; Misaligned (integer) moves: provide an implementation for
> > +;; movmisalign, so the default support_vector_misalignment() will
> > +;; return the right boolean depending on whether
> > +;; riscv_slow_unaligned_access_p is set or not.
> > +;;
> > +;; E.g., this is needed for SLP to convert "c[1] << 8) | c[0]" into a
> > +;; HImode load (a good test case will be blender and xalancbmk in SPEC
> > +;; CPU 2017).
> > +;;
> > +(define_expand "movmisalign<mode>"
> > +  [(set (match_operand:ANYI 0 "")
> > +       (match_operand:ANYI 1 ""))]
> > +  "!riscv_slow_unaligned_access_p"
> > +{
> > +  if (riscv_legitimize_move (<MODE>mode, operands[0], operands[1]))
> > +    DONE;
> > +})
> > +
> >  ;; 64-bit integer moves
> >
> >  (define_expand "movdi"
> > diff --git a/gcc/testsuite/gcc.target/riscv/movmisalign-1.c b/gcc/testsuite/gcc.target/riscv/movmisalign-1.c
> > new file mode 100644
> > index 00000000000..791a3d63335
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/movmisalign-1.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc -mabi=lp64 -mtune=size" } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > +
> > +void f(unsigned short *sink, unsigned char *arr)
> > +{
> > +  *sink = (arr[1] << 8) | arr[0];
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "lhu\t" 1 } } */
> > +/* { dg-final { scan-assembler-not "lbu\t" } } */
> > +
> > diff --git a/gcc/testsuite/gcc.target/riscv/movmisalign-2.c b/gcc/testsuite/gcc.target/riscv/movmisalign-2.c
> > new file mode 100644
> > index 00000000000..ef73dcb2d9d
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/movmisalign-2.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc -mabi=lp64 -mtune=size -mstrict-align" } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > +
> > +void f(unsigned short *sink, unsigned char *arr)
> > +{
> > +  *sink = (arr[1] << 8) | arr[0];
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "lbu\t" 2 } } */
> > +/* { dg-final { scan-assembler-not "lhu\t" } } */
> > +
> > diff --git a/gcc/testsuite/gcc.target/riscv/movmisalign-3.c b/gcc/testsuite/gcc.target/riscv/movmisalign-3.c
> > new file mode 100644
> > index 00000000000..963b11c27fd
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/movmisalign-3.c
> > @@ -0,0 +1,12 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-march=rv64gc -mabi=lp64 -mtune=rocket" } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-O1" } } */
> > +
> > +void f(unsigned short *sink, unsigned char *arr)
> > +{
> > +  *sink = (arr[1] << 8) | arr[0];
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "lbu\t" 2 } } */
> > +/* { dg-final { scan-assembler-not "lhu\t" } } */
> > +
> > --
> > 2.34.1
> >