Re: Ping ^ 2: [PATCH] rs6000: Remove unspecs for vec_mrghl[bhw]

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

Hi, Xionghu

What's the status of the \M and \m testcase beautification requested
by Segher?  Did you send an updated patch? Your messages ping the
version prior to Segher's additional comments.

It seems that the changes to the patterns are complete, but there are
remaining questions about the testcase style and if the instruction
counts are ideal. I trust that the instruction counts match the
behavior after the patch, but it seemed that Segher wanted to confirm
that the counts are the values desired / expected from optimal code
generation.  The counts are the total for the file, which doesn't
communicate if the sequences themselves are optimal.

Thanks, David

On Sun, Sep 5, 2021 at 8:54 PM Xionghu Luo <luoxhu@linux.ibm.com> wrote:
>
> Ping^2, thanks.
>
> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572330.html
>
>
> On 2021/6/30 09:47, Xionghu Luo via Gcc-patches wrote:
> > Gentle ping, thanks.
> >
> > https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572330.html
> >
> >
> > On 2021/6/9 16:03, Xionghu Luo via Gcc-patches wrote:
> >> Hi,
> >>
> >> On 2021/6/9 07:25, Segher Boessenkool wrote:
> >>> On Mon, May 24, 2021 at 04:02:13AM -0500, Xionghu Luo wrote:
> >>>> vmrghb only accepts permute index {0, 16, 1, 17, 2, 18, 3, 19, 4, 20,
> >>>> 5, 21, 6, 22, 7, 23} no matter for BE or LE in ISA, similarly for
> >>>> vmrghlb.
> >>>
> >>> (vmrglb)
> >>>
> >>>> +  if (BYTES_BIG_ENDIAN)
> >>>> +    emit_insn (
> >>>> +      gen_altivec_vmrghb_direct (operands[0], operands[1],
> >>>> operands[2]));
> >>>> +  else
> >>>> +    emit_insn (
> >>>> +      gen_altivec_vmrglb_direct (operands[0], operands[2],
> >>>> operands[1]));
> >>>
> >>> Please don't indent like that, it doesn't match what we do elsewhere.
> >>> For better or for worse (for worse imo), we use deep hanging indents.
> >>> If you have to, you can do something like
> >>>
> >>>    rtx insn;
> >>>    if (BYTES_BIG_ENDIAN)
> >>>      insn = gen_altivec_vmrghb_direct (operands[0], operands[1],
> >>> operands[2]);
> >>>    else
> >>>      insn = gen_altivec_vmrglb_direct (operands[0], operands[2],
> >>> operands[1]);
> >>>    emit_insn (insn);
> >>>
> >>> (this is better even, in that it has only one emit_insn), or even
> >>>
> >>>    rtx (*fun) () = BYTES_BIG_ENDIAN ? gen_altivec_vmrghb_direct
> >>>                    : gen_altivec_vmrglb_direct;
> >>>    if (!BYTES_BIG_ENDIAN)
> >>>      std::swap (operands[1], operands[2]);
> >>>    emit_insn (fun (operands[0], operands[1], operands[2]));
> >>>
> >>> Well, C++ does not allow that last example like that, sigh, so
> >>>    rtx (*fun) (rtx, rtx, rtx) = BYTES_BIG_ENDIAN ?
> >>> gen_altivec_vmrghb_direct
> >>>                         : gen_altivec_vmrglb_direct;
> >>>
> >>> This is shorter than the other two options ;-)
> >>
> >> Changed.
> >>
> >>>
> >>>> +(define_insn "altivec_vmrghb_direct"
> >>>>     [(set (match_operand:V16QI 0 "register_operand" "=v")
> >>>> +    (vec_select:V16QI
> >>>
> >>> This should be indented one space more.
> >>>
> >>>>     "TARGET_ALTIVEC"
> >>>>     "@
> >>>> -   xxmrghw %x0,%x1,%x2
> >>>> -   vmrghw %0,%1,%2"
> >>>> +  xxmrghw %x0,%x1,%x2
> >>>> +  vmrghw %0,%1,%2"
> >>>
> >>> The original indent was correct, please restore.
> >>>
> >>>> -      emit_insn (gen_altivec_vmrghw_direct (operands[0], ve, vo));
> >>>> +      emit_insn (gen_altivec_vmrghw_direct_v4si (operands[0], ve,
> >>>> vo));
> >>>
> >>> When you see a mode as part of a pattern name, chances are that it will
> >>> be a good candidate for using parameterized names with.  (But don't do
> >>> that now, just keep it in mind as a nice cleanup to do).
> >>
> >> OK.
> >>
> >>>
> >>>> @@ -23022,8 +23022,8 @@ altivec_expand_vec_perm_const (rtx target,
> >>>> rtx op0, rtx op1,
> >>>>          : CODE_FOR_altivec_vmrglh_direct),
> >>>>         {  0,  1, 16, 17,  2,  3, 18, 19,  4,  5, 20, 21,  6,  7,
> >>>> 22, 23 } },
> >>>>       { OPTION_MASK_ALTIVEC,
> >>>> -      (BYTES_BIG_ENDIAN ? CODE_FOR_altivec_vmrghw_direct
> >>>> -       : CODE_FOR_altivec_vmrglw_direct),
> >>>> +      (BYTES_BIG_ENDIAN ? CODE_FOR_altivec_vmrghw_direct_v4si
> >>>> +       : CODE_FOR_altivec_vmrglw_direct_v4si),
> >>>
> >>> The correct way is to align the ? and the : (or put everything on one
> >>> line of course, if that fits)
> >>>
> >>> The parens around this are not needed btw, and are a distraction.
> >>
> >> Changed.
> >>
> >>>
> >>>> --- a/gcc/testsuite/gcc.target/powerpc/builtins-1.c
> >>>> +++ b/gcc/testsuite/gcc.target/powerpc/builtins-1.c
> >>>> @@ -317,10 +317,10 @@ int main ()
> >>>>   /* { dg-final { scan-assembler-times "vctuxs" 2 } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrghb" 4 { target be } } } */
> >>>> -/* { dg-final { scan-assembler-times "vmrghb" 5 { target le } } } */
> >>>> +/* { dg-final { scan-assembler-times "vmrghb" 6 { target le } } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrghh" 8 } } */
> >>>> -/* { dg-final { scan-assembler-times "xxmrghw" 8 } } */
> >>>> -/* { dg-final { scan-assembler-times "xxmrglw" 8 } } */
> >>>> +/* { dg-final { scan-assembler-times "xxmrghw" 4 } } */
> >>>> +/* { dg-final { scan-assembler-times "xxmrglw" 4 } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrglh" 8 } } */
> >>>>   /* { dg-final { scan-assembler-times "xxlnor" 6 } } */
> >>>>   /* { dg-final { scan-assembler-times {\mvpkudus\M} 1 } } */
> >>>> @@ -347,7 +347,7 @@ int main ()
> >>>>   /* { dg-final { scan-assembler-times "vspltb" 6 } } */
> >>>>   /* { dg-final { scan-assembler-times "vspltw" 0 } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrgow" 8 } } */
> >>>> -/* { dg-final { scan-assembler-times "vmrglb" 5 { target le } } } */
> >>>> +/* { dg-final { scan-assembler-times "vmrglb" 4 { target le } } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrglb" 6 { target be } } } */
> >>>>   /* { dg-final { scan-assembler-times "vmrgew" 8 } } */
> >>>>   /* { dg-final { scan-assembler-times "vsplth" 8 } } */
> >>>
> >>> Are those changes correct?  It looks like a vmrglb became a vmrghb, and
> >>> that 4 each of xxmrghw and xxmrglw disappeared?  Both seem wrong?
> >>
> >>
> >> This case is built with "-mdejagnu-cpu=power8 -O0 -mno-fold-gimple -dp"
> >> and it also counted the generated instruction patterns.
> >>
> >> 1) "vsx_xxmrghw_v4si" is replaced by "altivec_vmrglw_direct_v4si/0",
> >> so it decreases from 8 to 4. (Likewise for vsx_xxmrglw_v4si.)
> >>
> >>          li 9,48          # 1282 [c=4 l=4]  *movdi_internal64/3
> >> -       lxvd2x 0,31,9    # 31   [c=8 l=4]  *vsx_lxvd2x4_le_v4si
> >> -       xxpermdi 0,0,0,2         # 32   [c=4 l=4]  xxswapd_v4si
> >> -       xxmrglw 0,0,12   # 33   [c=4 l=4]  vsx_xxmrghw_v4si
> >> +       lxvd2x 12,31,9   # 31   [c=8 l=4]  *vsx_lxvd2x4_le_v4si
> >> +       xxpermdi 12,12,12,2      # 32   [c=4 l=4]  xxswapd_v4si
> >> +       xxmrglw 0,12,0   # 33   [c=4 l=4]  altivec_vmrglw_direct_v4si/0
> >>          xxpermdi 0,0,0,2         # 35   [c=4 l=4]  xxswapd_v4sf
> >>
> >> Note that v0 and v12 is swapped in lxvd2x, these new 3 instructions
> >> produces same result than before.
> >>
> >> 2) "*altivec_vmrglb_internal" is replaced by "altivec_vmrghb_direct"
> >> with this patch, then vmrglb count decreases from 5 to 4 and vmrghb
> >> increases from 5 to 6. (BYTES_BIG_ENDIAN is checked early in RTL
> >> generation instead of final to remove the UNSPECs for potential
> >> optimization through backend.)
> >>
> >>          li 9,928                 # 1424 [c=4 l=4]  *movdi_internal64/3
> >>          lxvd2x 0,31,9    # 416  [c=8 l=4]  *vsx_lxvd2x16_le_V16QI
> >> -       xxpermdi 33,0,0,2        # 417  [c=4 l=4]  xxswapd_v16qi
> >> +       xxpermdi 32,0,0,2        # 417  [c=4 l=4]  xxswapd_v16qi
> >>          li 9,944                 # 1425 [c=4 l=4]  *movdi_internal64/3
> >>          lxvd2x 0,31,9    # 418  [c=8 l=4]  *vsx_lxvd2x16_le_V16QI
> >> -       xxpermdi 32,0,0,2        # 419  [c=4 l=4]  xxswapd_v16qi
> >> -       vmrghb 0,0,1     # 420  [c=4 l=4]  *altivec_vmrglb_internal
> >> +       xxpermdi 33,0,0,2        # 419  [c=4 l=4]  xxswapd_v16qi
> >> +       vmrghb 0,1,0     # 420  [c=4 l=4]  altivec_vmrghb_direct
> >>          xxpermdi 0,32,32,2       # 421  [c=4 l=4]  xxswapd_v16qi
> >>
> >> Seems not necessary to also use \m and \M here to count only ASM here?
> >> Update the patch as attached.
> >>
> >
>
> --
> Thanks,
> Xionghu