Re: [PATCH v3] rs6000: Use direct move for char/short vector CTOR [PR96933]

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Tue, Nov 03, 2020 at 03:25:19PM +0800, Kewen.Lin wrote:
> > I'm trying to be stricter about the test cases.
> > 
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr96933-1.c
> > @@ -0,0 +1,14 @@
> > +/* { dg-do compile { target { lp64 && has_arch_pwr9 } } } */
> > +/* { dg-require-effective-target powerpc_p9vector_ok } */
> > +/* { dg-options "-O2" } */
> > 
> > Why does this test has_arch_pwr9 instead of adding -mdejagnu-cpu=power9?
> 
> I thought using -mdejagnu-cpu=power9 would force the case run with
> power9 cpu all the time, while using has_arch_pwr9 seems to be more
> flexible, it can be compiled with power9 or later (like -mcpu=power10),
> we can check whether we generate unexpected code on power10 or later.
> Does it sound good?

It will not run at all if your compiler (or testsuite invocation) does
not use at least power9.  Since the default for powerpc64-linux is
power4, and that for powerpc64le-linux is power8, this will happen for
many people (not to mention that it is extra important to test the
default setup, of course).

It probably would be useful if there was some convenient way to say
"use at least -mcpu=power9 for this, but some later cpu is fine too" --
but there is no such thing yet.

Using something like that might cause more maintenance issues later, see
"pstb" below for example, but that is not really an argument against
fixing this.

> > +++ b/gcc/testsuite/gcc.target/powerpc/pr96933-3.c
> > @@ -0,0 +1,63 @@
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target p8vector_hw } */
> > +/* { dg-options "-O2" } */
> > 
> > Doesn't this need -mdejagnu-cpu=power8?
> 
> Thanks for catching!  Yes, it needs.  I was thinking to use one
> case for both Power8 and Power9 runs, it passed the testings on
> both machines.  But your question made me realize that it's
> incorrect when we are doing testing on Power8 but pass some
> external option like -mcpu=power9, it can generate power9 insns
> which are illegal on the machine.

If the compiler defaults to (say) -mcpu=power7, it will generate code
for that the way the testcase is set up now (and it will not run on
machines before power8, but that is separate).

> +  if (TARGET_DIRECT_MOVE && (mode == V16QImode || mode == V8HImode))
> +    {
> +      rtx op[16];
> +      /* Force the values into word_mode registers.  */
> +      for (i = 0; i < n_elts; i++)
> +	{
> +	  rtx tmp = force_reg (GET_MODE_INNER (mode), XVECEXP (vals, 0, i));
> +	  if (TARGET_POWERPC64)
> +	    {
> +	      op[i] = gen_reg_rtx (DImode);
> +	      emit_insn (gen_zero_extendqidi2 (op[i], tmp));
> +	    }
> +	  else
> +	    {
> +	      op[i] = gen_reg_rtx (SImode);
> +	      emit_insn (gen_zero_extendqisi2 (op[i], tmp));
> +	    }
> +	}

TARGET_POWERPC64 should be TARGET_64BIT afaics?  (See below.)

You can use Pmode then, too.  The zero_extend thing can be handled by
changing
  (define_insn "zero_extendqi<mode>2"
to
  (define_insn "@zero_extendqi<mode>2"
(and no other changes needed), and then calling
  emit_insn (gen_zero_extendqi2 (Pmode, op[i], tmp));
(or so is the theory.  This might need some other changes, and also all
other gen_zero_extendqi* callers need to change, so that is a separate
patch if you want to try.  This isn't so bad right now.)

> +	  for (i = 0; i < n_elts; i++)
> +	    {
> +	      vr_qi[i] = gen_reg_rtx (V16QImode);
> +	      if (TARGET_POWERPC64)
> +		emit_insn (gen_p8_mtvsrd_v16qidi2 (vr_qi[i], op[i]));
> +	      else
> +		emit_insn (gen_p8_mtvsrwz_v16qisi2 (vr_qi[i], op[i]));
> +	    }

TARGET_64BIT here as well.

TARGET_POWERPC64 means the current machine has the 64-bit insns.  It
does not mean the code will run in 64-bit mode (e.g. -m32 -mpowerpc64 is
just fine, and can be useful), but it also does not mean the OS (libc,
kernel, etc.) will actually save the full 64-bit registers -- making it
only useful on Darwin currently.

(You *can* run all of the testsuite flawlessly on Linux with those
options, but that only works because those are small, short-running
programs.  More "real", bigger and more complex programs fail in strange
and exciting ways!)

It's a pity the pre-p9 code cannot reuse most of what we do for p9.

> +(define_insn "p8_mtvsrwz_v16qisi2"
> +  [(set (match_operand:V16QI 0 "register_operand" "=wa")
> +    (unspec:V16QI [(match_operand:SI 1 "register_operand" "r")]
> +		  UNSPEC_P8V_MTVSRWZ))]
> +  "!TARGET_POWERPC64 && TARGET_DIRECT_MOVE"
> +  "mtvsrwz %x0,%1"
> +  [(set_attr "type" "mftgpr")])
> +
> +(define_insn "p8_mtvsrd_v16qidi2"
> +  [(set (match_operand:V16QI 0 "register_operand" "=wa")
> +    (unspec:V16QI [(match_operand:DI 1 "register_operand" "r")]
> +		  UNSPEC_P8V_MTVSRD))]
> +  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE"
> +  "mtvsrd %x0,%1"
> +  [(set_attr "type" "mftgpr")])

TARGET_POWERPC64 is fine for these, btw.  You just cannot decide to put
a DImode in a register based on only this -- but if that has been
decided already, it is just fine.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr96933-1.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target { lp64 && has_arch_pwr9 } } } */
> +/* { dg-require-effective-target powerpc_p9vector_ok } */
> +/* { dg-options "-O2" } */

As David said:

/* { dg-do compile } */
/* { dg-require-effective-target lp64 } */
/* { dg-require-effective-target has_arch_pwr9 } */
/* { dg-require-effective-target powerpc_p9vector_ok } */
/* { dg-options "-O2 -mdejagnu-cpu=power9" } */

But, you probably don't want that has_arch_pwr9 line at all, this is a
compile test?

> +/* { dg-final { scan-assembler-not {\mstb\M} } } */
> +/* { dg-final { scan-assembler-not {\msth\M} } } */

Btw, if this didn't for -mcpu=power9, you probably would need to allow
prefixed stores here, like  {\mp?stb\M} .

So, okay for trunk with those TARGET_POWERPC64 fixed, and that one
remaining testcase.  Thanks!


Segher