Re: [PATCH] rs6000: Define movsf_from_si2 to extract high part SF element from DImode[PR89310]

[David Edelsohn <dje.gcc@gmail.com>]

Unfortunately this patch is eliciting a number of new testsuite
failures, all like

error: unrecognizable insn:
(insn 44 43 45 5 (parallel [
            (set (reg:SI 199)
                (unspec:SI [
                        (reg:SF 202)
                    ] UNSPEC_SI_FROM_SF))
            (clobber (scratch:V4SF))
        ]) "/nasfarm/edelsohn/src/src/gcc/testsuite/gcc.dg/vect/vect-alias-check-11.c":70:1
-1
     (nil))
during RTL pass: vregs

for

gcc.dg/vect/vect-alias-check-11.c
gcc.dg/vect/vect-alias-check-12.c
gcc.dg/vect/pr57741-2.c
gcc.dg/vect/pr57741-3.c
gcc.dg/vect/pr89440.c
gcc.target/powerpc/sse-movss-1.c

Thanks, David

On Mon, Jul 13, 2020 at 2:30 AM luoxhu <luoxhu@linux.ibm.com> wrote:
>
> Hi,
>
> On 2020/7/11 08:54, Segher Boessenkool wrote:
> > Hi!
> >
> > On Fri, Jul 10, 2020 at 09:39:40AM +0800, luoxhu wrote:
> >> OK, seems the md file needs a format tool too...
> >
> > Heh.  Just make sure it looks good (that is, does what it looks like),
> > looks like the rest, etc.  It's hard to do anything nice with unspecs,
> > [ ] lists do not format well.
> >
> >>>> +  "TARGET_NO_SF_SUBREG"
> >>>> +  "#"
> >>>> +  "&& vsx_reg_sfsubreg_ok (operands[0], SFmode)"
> >>>
> >>> Put this in the insn condition?  And since this is just a predicate,
> >>> you can just use it instead of gpc_reg_operand.
> >>>
> >>> (The split condition becomes "&& 1" then, not "").
> >>
> >> OK, this seems a bit strange as movsi_from_sf, movsf_from_si and
> >> movdi_from_sf_zero_ext all use it as condition...
> >
> > Since in your case you *always* split, the split condition should be
> > "always".  There are no alternatives that do not split here.
> >
> >> And why vsx_reg_sfsubreg_ok allows "SF SUBREGS" and TARGET_NO_SF_SUBREG
> >> "avoid (SUBREG:SF (REG:SI)", I guess they are not the same meaning? (The
> >> TARGET_NO_SF_SUBREG is also copied from other similar defines.)  Thanks.
> >
> > Good question.  I do not know.
> >
> > Well...  Since this define_insn* requires p8 *anyway*, we do not need
> > any of these sf_subreg things?  We always know for each one if it should
> > be true or false.
>
> Yes, removed the vsx_reg_sfsubreg_ok check.
>
> >
> >> +  "TARGET_NO_SF_SUBREG"
> >
> > But here we should require p8 some other way, then.
>
> TARGET_NO_SF_SUBREG is defined to TARGET_DIRECT_MOVE_64BIT, and
> TARGET_DIRECT_MOVE_64BIT is TARGET_DIRECT_MOVE && TARGET_P8_VECTOR && TARGET_POWERPC64
> which means TARGET_P8_VECTOR must be true for TARGET_NO_SF_SUBREG.
>
> >
> >> +  (set_attr "isa" "p8v")])
> >
> > (This isn't enough, unfortunately).
> >
>
>
> Updated patch to removed the vsx_reg_sfsubreg_ok and ICE fix:
>
>
> For extracting high part element from DImode register like:
>
> {%1:SF=unspec[r122:DI>>0x20#0] 86;clobber scratch;}
>
> split it before reload with "and mask" to avoid generating shift right
> 32 bit then shift left 32 bit.  This pattern also exists in PR42475 and
> PR67741, etc.
>
> srdi 3,3,32
> sldi 9,3,32
> mtvsrd 1,9
> xscvspdpn 1,1
>
> =>
>
> rldicr 3,3,0,31
> mtvsrd 1,3
> xscvspdpn 1,1
>
> Bootstrap and regression tested pass on Power8-LE.
>
> gcc/ChangeLog:
>
> 2020-07-13  Xionghu Luo  <luoxhu@linux.ibm.com>
>
>         PR rtl-optimization/89310
>         * config/rs6000/rs6000.md (movsf_from_si2): New
>         define_insn_and_split.
>
> gcc/testsuite/ChangeLog:
>
> 2020-07-13  Xionghu Luo  <luoxhu@linux.ibm.com>
>
>         PR rtl-optimization/89310
>         * gcc.target/powerpc/pr89310.c: New test.
> ---
>  gcc/config/rs6000/rs6000.md                | 31 ++++++++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/pr89310.c | 17 ++++++++++++
>  2 files changed, 48 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr89310.c
>
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 4fcd6a94022..480385ed4d2 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -7593,6 +7593,37 @@ (define_insn_and_split "movsf_from_si"
>             "*,          *,         p9v,       p8v,       *,         *,
>              p8v,        p8v,       p8v,       *")])
>
> +;; For extracting high part element from DImode register like:
> +;;     {%1:SF=unspec[r122:DI>>0x20#0] 86;clobber scratch;}
> +;; split it before reload with "and mask" to avoid generating shift right
> +;; 32 bit then shift left 32 bit.
> +(define_insn_and_split "movsf_from_si2"
> +  [(set (match_operand:SF 0 "gpc_reg_operand" "=wa")
> +           (unspec:SF
> +            [(subreg:SI
> +              (ashiftrt:DI
> +               (match_operand:DI 1 "input_operand" "r")
> +               (const_int 32))
> +              0)]
> +            UNSPEC_SF_FROM_SI))
> +  (clobber (match_scratch:DI 2 "=r"))]
> +  "TARGET_NO_SF_SUBREG"
> +  "#"
> +  "&& 1"
> +  [(const_int 0)]
> +{
> +  if (GET_CODE (operands[2]) == SCRATCH)
> +    operands[2] = gen_reg_rtx (DImode);
> +
> +  rtx mask = GEN_INT (HOST_WIDE_INT_M1U << 32);
> +  emit_insn (gen_anddi3 (operands[2], operands[1], mask));
> +  emit_insn (gen_p8_mtvsrd_sf (operands[0], operands[2]));
> +  emit_insn (gen_vsx_xscvspdpn_directmove (operands[0], operands[0]));
> +  DONE;
> +}
> +  [(set_attr "length" "12")
> +  (set_attr "type" "vecfloat")
> +  (set_attr "isa" "p8v")])
>
>  ;; Move 64-bit binary/decimal floating point
>  (define_expand "mov<mode>"
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr89310.c b/gcc/testsuite/gcc.target/powerpc/pr89310.c
> new file mode 100644
> index 00000000000..15e78509246
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr89310.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +struct s {
> +  int i;
> +  float f;
> +};
> +
> +float
> +foo (struct s arg)
> +{
> +  return arg.f;
> +}
> +
> +/* { dg-final { scan-assembler-not {\msrdi\M} } } */
> +/* { dg-final { scan-assembler-not {\msldi\M} } } */
> +/* { dg-final { scan-assembler-times {\mrldicr\M} 1 } } */
> --
> 2.21.0.777.g83232e3864
>
>