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

[Maryam Moghadas via Phabricator via llvm-commits]

maryammo added inline comments.
Herald added a subscriber: wangpc.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10238
       NeedSwap = !NeedSwap;
     }
   }
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149083