Re: [PATCH 3/4] Make vsx_extract_<mode> use correct insn attributes, PR target 99293.

[Michael Meissner <meissner@linux.ibm.com>]

On Mon, Mar 28, 2022 at 05:06:00PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Mon, Mar 28, 2022 at 12:28:04PM -0400, Michael Meissner wrote:
> > In looking at PR target/99293, I noticed that the insn "type" attribute is
> > incorrect for the vsx_extract_<mode> insns.  In particular:
> > 
> >     1)	Simple vector register move should be vecmove (alternative 1);
> >     2)	Xxpermdi should be vecperm (alternative 2); (and)
> >     3)	Mfvsrld should be mfvsr (alternative 4).
> > 
> > This patch fixes those attributes.
> 
> But the code does not correspond well to the alternatives, even worse
> for BE.  It would be much clearer (and even possibly correct!) if it
> would just use the alternative # in the code, instead of using twenty
> different conditions.  There are some important cases that have no
> alternative right now, like, when op 1 is the same as op 0: it should
> have the constraint "0" for op 1 then, and have cost 0.  The register
> allocator will then hopefully try to make that happen.
> 
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -3451,7 +3451,7 @@ (define_insn "vsx_extract_<mode>"
> >    else
> >      gcc_unreachable ();
> >  }
> > -  [(set_attr "type" "veclogical,mfvsr,mfvsr,vecperm")
> > +  [(set_attr "type" "vecmove,vecperm,mfvsr,mfvsr")
> >     (set_attr "isa" "*,*,p8v,p9v")])
> 
> The generated code is one of
>   no-op
>   mfvsrd
>   fmr
>   xxlor
>   mfvsrld
>   xxpermdi
> 
> Which of the 4 alts are meant to correspond to which of those six
> possible generated pieces of code?

The mop comment, fmr, and xxlor are all alternative 1 (target is a VSX
register, element number is which double-word that that is in the upper 64-bits
of the register).

The xxpermdi is alternative 2 (target is a VSX register, elment number is which
double-word that is in the lower 64-bits of the register).

The mfvsrd is alternative 3 (target is a GPR register, element number is which
double-word that is in the upper 64-bits of the register).

The mfvsrld is alternative 4 (target is a GPR register, elment number is which
double-word that is in the lower 64-bits of the register).

In terms of this patch, I had originally rewritten the insn as a
define_insn_and_split, and changed the move case into just a move to let the
later passes just delete moves to the same register.  However, it caused some
unrelated errors (in the gcc.dg/tree-ssa/builtin-s*printf tests), so I backed
out the code to use split to create moves.

But it looks like alternative 2 and alternative 4 have the insn attribute
"type" mixed up (alternative 2 should be vecperm, alternative 4 should be move
from vsr).



-- 
Michael Meissner, IBM
PO Box 98, Ayer, Massachusetts, USA, 01432
email: meissner@linux.ibm.com