[Bug target/106770] New: PPCLE: Unnecessary xxpermdi before mfvsrd

[Bug target/106770] New: PPCLE: Unnecessary xxpermdi before mfvsrd

[jens.seifert at de dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

            Bug ID: 106770
           Summary: PPCLE: Unnecessary xxpermdi before mfvsrd
           Product: gcc
           Version: 11.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jens.seifert at de dot ibm.com
  Target Milestone: ---

#include <altivec.h>

int cmp2(double a, double b)
{
    vector double va = vec_promote(a, 1);
    vector double vb = vec_promote(b, 1);
    vector long long vlt = (vector long long)vec_cmplt(va, vb);
    vector long long vgt = (vector long long)vec_cmplt(vb, va);
    vector signed long long vr = vec_sub(vlt, vgt);

    return vec_extract(vr, 1);
}

Generates:

_Z4cmp2dd:
.LFB1:
        .cfi_startproc
        xxpermdi 1,1,1,0
        xxpermdi 2,2,2,0
        xvcmpgtdp 33,2,1
        xvcmpgtdp 32,1,2
        vsubudm 0,1,0
        xxpermdi 0,32,32,3
        mfvsrd 3,0
        extsw 3,3
        blr

The unnecessary xxpermdi for vec_promote are already reported in another
bugzilla case.

mfvsrd can access all 64 vector registers directly and xxpermdi is not
required.
mfvsrd 3,32 expected instead xxpermdi 0,32,32,3 + mfvsrd 3,0

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[jens.seifert at de dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #1 from Jens Seifert <jens.seifert at de dot ibm.com> ---
vec_extract(vr, 1) should extract the left element. But xxpermdi x,x,x,3
extracts the right element.
Looks like a bug in vec_extract for PPCLE and not a problem regarding
unnecessary xxpermdi.

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[jens.seifert at de dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #2 from Jens Seifert <jens.seifert at de dot ibm.com> ---
vec_extract(vr, 1) should extract the left element. But xxpermdi x,x,x,3
extracts the right element.
Looks like a bug in vec_extract for PPCLE and not a problem regarding
unnecessary xxpermdi.

Using assembly for the subtract:
int cmp3(double a, double b)
{
    vector double va = vec_promote(a, 0);
    vector double vb = vec_promote(b, 0);
    vector long long vlt = (vector long long)vec_cmplt(va, vb);
    vector long long vgt = (vector long long)vec_cmplt(vb, va);
    vector signed long long vr;
    __asm__ volatile("vsubudm %0,%1,%2":"=v"(vr):"v"(vlt),"v"(vgt):);
    //vector signed long long vr = vec_sub(vlt, vgt);

    return vec_extract(vr, 1);
}

generates:

_Z4cmp3dd:
.LFB2:
        .cfi_startproc
        xxpermdi 1,1,1,0
        xxpermdi 2,2,2,0
        xvcmpgtdp 32,2,1
        xvcmpgtdp 33,1,2
#APP
 # 34 "cmpdouble.C" 1
        vsubudm 0,0,1
 # 0 "" 2
#NO_APP
        mfvsrd 3,32
        extsw 3,3
"

Looks like the compile knows about the vec_promote doing splat and at the end
extracts the non-preferred right element instead of the expected left element.

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |segher at gcc dot gnu.org

--- Comment #3 from Segher Boessenkool <segher at gcc dot gnu.org> ---
You get xxpermdi ,3 only with -mlittle.  Maybe you mean this is for target
powerpc64le-linux?  That is very different from powerpcle-linux fwiw, don't
confuse those :-)

What -mcpu= did you use?

xxpermdi doesn't extract any element of course, it is mfvsr[l]d that does
that.  But duplicating the element you extract isn't needed, sure.

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[jens.seifert at de dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #4 from Jens Seifert <jens.seifert at de dot ibm.com> ---
PPCLE with no special option means -mcpu=power8 -maltivec  (altivecle to be mor
precise).

vec_promote(<double value>, 1) should be a noop on ppcle. But value gets
splatted to both left and right part of vector register. => 2 unnecesary
xxpermdi
The rest of the operations are done on left and right part.

vec_extract(<vector double>, 1) should be noop on ppcle. But value gets taken
from right part of register which requires a xxpermdi

Overall 3 unnecessary xxpermdi. Don't know why the right part of register gets
"preferred".

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #5 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jens Seifert from comment #4)
> PPCLE with no special option means -mcpu=power8 -maltivec  (altivecle to be
> mor precise).

What?  No.

$ sh config.sub ppcle      
powerpcle-unknown-none

This is typically the old 32-bit PowerPC ELF format.  powerpcle-elf (which
non-canonically can be called ppcle-elf) for example, or ppcle-linux, but not
ppcle-aix and the like (that one doesn't even exist; at least one COFF
format has existed in the past though).

This may not matter for you, but it is awfully confusing for others.


powerpc64le-linux (and I believe all other existing ELFv2 ports) require
a p8 or later CPU, sure; but it is perfectly valid to have no AltiVec even
then, or for a port to default to some other CPU.  Currently we have no such
thing, and all default defaults are like you say, but that might change.

> vec_promote(<double value>, 1) should be a noop on ppcle.

It never is, not on powerpc64le either.  It always duplicates the selected
elt to all lanes.

> But value gets
> splatted to both left and right part of vector register. => 2 unnecesary
> xxpermdi

So why are those not optimised away?  *That* is the question!

> The rest of the operations are done on left and right part.
> 
> vec_extract(<vector double>, 1) should be noop on ppcle. But value gets
> taken from right part of register which requires a xxpermdi
> 
> Overall 3 unnecessary xxpermdi. Don't know why the right part of register
> gets "preferred".

I don't know what you mean there?  The ABIs say where parameters and return
values are stored, but you mean something else?

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[jens.seifert at de dot ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #6 from Jens Seifert <jens.seifert at de dot ibm.com> ---
The left part of VSX registers overlaps with floating point registers, that is
why no register xxpermdi is required and mfvsrd can access all (left) parts of
VSX registers directly.
The xxpermdi x,y,y,3 indicates to me that gcc prefers right part of register
which might also cause the xxpermdi at the beginning. At the end the mystery is
why gcc adds 3 xxpermdi to the code.

[Bug target/106770] PPCLE: Unnecessary xxpermdi before mfvsrd

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

Segher Boessenkool <segher at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
             Target|powerpc                     |powerpc64le
   Last reconfirmed|                            |2023-02-28
     Ever confirmed|0                           |1

--- Comment #7 from Segher Boessenkool <segher at gcc dot gnu.org> ---
The mystery is not where the permutations came from: they were added during
expand to make correct code, just like many unnecessary register moves are
added at that time.  This is normal, and even good in many ways.

The question is why they weren't optimised better.  This is either due to
some bug, or this is an enhancement request.

[Bug target/106770] powerpc64le: Unnecessary xxpermdi before mfvsrd

[jskumari at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #8 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
While the first two xxpermdi's are fine, the 3rd one is a bug. It is incorrect.
Here is the C code inlined into assembly:

_Z4cmp2dd:
.LFB1:
        .cfi_startproc

           // vector double va = vec_promote(a, 1);
        xxpermdi 1,1,1,0      

           // vector double vb = vec_promote(b, 1);
        xxpermdi 2,2,2,0

           // vector long long vlt = (vector long long)vec_cmplt(va, vb);
        xvcmpgtdp 33,2,1

           // vector long long vgt = (vector long long)vec_cmplt(vb, va);
        xvcmpgtdp 32,1,2

           // vector signed long long vr = vec_sub(vlt, vgt);
        vsubudm 0,1,0

           // return vec_extract(vr, 1);
        xxpermdi 0,32,32,3
        mfvsrd 3,0
        extsw 3,3
        blr

The code generated for vec_extract(vr, 1) is incorrect. We want to extract the
1st element but 'xxpermdi 0,32,32,3' copies the 0th element of vsx32 into the
0th and 1st elements of vsx0. The mfvsrd instruction does extract the 1st
element, but this is not the first element of the result of the vec_sub.

[Bug target/106770] powerpc64le: Unnecessary xxpermdi before mfvsrd

[jskumari at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #9 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
RTL after dfinit pass for the vec_sub() and the vec_extract():

(insn 13 12 14 2 (set (reg:V2DI 132 [ vrD.3952 ])
        (minus:V2DI (subreg:V2DI (reg:V2DF 117 [ _1 ]) 0)
            (subreg:V2DI (reg:V2DF 118 [ _2 ]) 0))) "cpm2.c":9:29 1689
{subv2di3}
     (nil))
(insn 14 13 15 2 (set (reg:DI 133)
        (vec_select:DI (reg:V2DI 132 [ vrD.3952 ])
            (parallel [
                    (const_int 1 [0x1])
                ]))) "cpm2.c":11:12 1371 {*vsx_extract_v2di_0}
     (nil))
(insn 15 14 16 2 (set (reg:DI 119 [ _3 ])
        (reg:DI 133)) "cpm2.c":11:12 679 {*movdi_internal64}
     (nil))
(insn 16 15 17 2 (set (reg:SI 134)
        (subreg:SI (reg:DI 119 [ _3 ]) 0)) "cpm2.c":11:12 discrim 1 555
{*movsi_internal1}
     (nil))
(insn 17 16 18 2 (set (reg:DI 135)
        (sign_extend:DI (reg:SI 134))) "cpm2.c":11:12 discrim 1 31
{extendsidi2}
     (nil))
(insn 18 17 22 2 (set (reg:DI 127 [ <retval> ])
        (reg:DI 135)) "cpm2.c":11:12 discrim 1 679 {*movdi_internal64}
     (nil))
(insn 22 18 23 2 (set (reg/i:DI 3 3)
        (reg:DI 127 [ <retval> ])) "cpm2.c":12:1 679 {*movdi_internal64}
     (nil))
(insn 23 22 0 2 (use (reg/i:DI 3 3)) "cpm2.c":12:1 -1
     (nil))

----------

RTL after swaps pass:

(insn 13 12 14 2 (set (reg:V2DI 132 [ vrD.3952 ])
        (minus:V2DI (subreg:V2DI (reg:V2DF 117 [ _1 ]) 0)
            (subreg:V2DI (reg:V2DF 118 [ _2 ]) 0))) "cpm2.c":9:29 1689
{subv2di3}
     (nil))
(insn 14 13 15 2 (set (reg:DI 133)
        (vec_select:DI (reg:V2DI 132 [ vrD.3952 ])
            (parallel [
                    (const_int 0 [0])
                ]))) "cpm2.c":11:12 -1
     (nil))

----------

'swaps' pass occurs after 'dfinit' pass.
After dfinit pass, we are extracting the correct element (1st element). See the
vec_select in insn 14. But after 'swaps' pass, the element being extracted
changes to 0.

[Bug target/106770] powerpc64le: Unnecessary xxpermdi before mfvsrd

[jskumari at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #10 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
The swap pass analyzes vector computations and removes unnecessary doubleword
swaps (xxswapdi instructions). The swap pass first constructs webs and removes
swap instructions if possible. If the web contains operations that are
sensitive to element order, such as an extract, then such instructions should
be modified. For example, the lane is changed on an extract operation.

As we saw in comment 9, the swap pass has incorrectly changed the lane of the
vec_extract. The swap pass modifies the extract operation even though there are
no swap instructions in the web. This is a bug in the swap pass. It should
modify only those operations which are present in webs having swap
instructions.

[Bug target/106770] powerpc64le: Unnecessary xxpermdi before mfvsrd

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #11 from Segher Boessenkool <segher at gcc dot gnu.org> ---
(In reply to Jens Seifert from comment #6)
> The left part of VSX registers overlaps with floating point registers, that
> is why no register xxpermdi is required and mfvsrd can access all (left)
> parts of VSX registers directly.

The mfvsrd instruction was invented before ELFv2 (at the same time as mfvsrwz).
Everything in common use was big-endian then.  The insns to move GPR->VSR that
initially existed were mtvstrd and mtvsrw[az], all of which write to dword 0 of
the target VSR.

Dword 0 of vector regs is where 64-bit entities in vector regs are stored in
the ABIs, sure, and that corresponds to the FPRs in the ISA.  mtvsrdd and
mtvsrws
were added in ISA 3.0 (p9), together with mfvsrld, to make little-endian work
better with little-endian ELFv2.

> The xxpermdi x,y,y,3 indicates to me that gcc prefers right part of register
> which might also cause the xxpermdi at the beginning.

And with -mbig you get ,2 here.  It is accidental.

> At the end the mystery
> is why gcc adds 3 xxpermdi to the code.

As I said, this is constructed during expand, to make correct code.  That is
all
that expand should do: make correct (and well-optimisable, "open structured",
easy to transform, code).  We should be able to optimise this to something
better in later passes that *are* supposed to make faster code.  Like the p8
swaps pass, which mostly zaps unnecessary pairs of swaps, or the swiss army
bazooka combine, or even many earlier passes if such an xxpermdi insn is truly
superfluous.  It usually is not, we are dealing with the full 128-bit VSRs
there, there is no way of saying we do not care about part of the register
contents.  Making infra for that is big work.

We can make things easier by expressing things as 64 bit earlier.  We can (and
should) also investigate why the mfvsrd is not combined (as in, what the
instruction combiner pass does) with the xxpermdi.  There are many things not
quite perfect here.

[Bug target/106770] powerpc64le: Unnecessary xxpermdi before mfvsrd

[jskumari at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106770

--- Comment #12 from Surya Kumari Jangala <jskumari at gcc dot gnu.org> ---
(In reply to Jens Seifert from comment #6)
> The left part of VSX registers overlaps with floating point registers, that
> is why no register xxpermdi is required and mfvsrd can access all (left)
> parts of VSX registers directly.
> The xxpermdi x,y,y,3 indicates to me that gcc prefers right part of register
> which might also cause the xxpermdi at the beginning. At the end the mystery
> is why gcc adds 3 xxpermdi to the code.

The 3rd xxpermdi is incorrect.