[PATCH] D149083: [PowerPC] Optimize VPERM and fix code order for swapping vector operands on LE

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10238
       NeedSwap = !NeedSwap;
     }
   }
----------------
maryammo wrote:
> stefanp wrote:
> > maryammo wrote:
> > > stefanp wrote:
> > > > Now that the swap has been moved down should this also be moved down? Or at least part of it?
> > > > I ask because I've noticed that there are a number of tests where a new `vmr` has been introduced and I'm wondering if that's why.
> > > In the following code section (line 10286), it uses the `NeedSwap` to adjust the mask, so this single-use operand swap needs to happen before that. 
> > > You are right, there are new `vmr`s, they seem to be needed based on the new vector operand order in the `xxperm` instruction. 
> > I see what you are saying. However, maybe something like this will work:
> > ```
> > if ((!isLittleEndian && !V2->hasOneUse() && V1->hasOneUse()) ||
> >         (isLittleEndian && !V1->hasOneUse() && V2->hasOneUse())) {
> > ```
> > I tried the above on one of the tests with the extra copy and it seems to get rid of it. I have not tested this extensively so please make sure that I have not broken anything.
> I tried this by adding your code here and removing
> 
> ```
>   // Only need to place items backwards in LE,
>   // the mask was properly calculated.
>   if (isLittleEndian)
>     std::swap(V1, V2);
> ```
> from line 10334. It regressed bunch of test cases, looking at them I realized we can not check the endianness here because this block of code is already guarded by 
> 
> 
> ```
> if (Subtarget.hasVSX() && Subtarget.hasP9Vector() &&
>       (V1->hasOneUse() || V2->hasOneUse())) {
> ```
> while this check  `if (isLittleEndian)` is for all subtargets. 
I think I didn't explain this properly.

I think that you should leave the code for:
```
// Only need to place items backwards in LE,
// the mask was properly calculated.
if (isLittleEndian)
  std::swap(V1, V2);
```
at the end. That accounts for Big vs. Little Endian and as you said should be for all subtargets. 

What I was thinking is just changing this if statement here. This swap is only here to try to prevent copying registers. What I'm proposing is that you take into consideration whether or not there will be a final swap at the end when you check if you want to also do this swap here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149083/new/

https://reviews.llvm.org/D149083