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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Overall I think that this makes sense to me. I do have a couple of comments but mostly nits.



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


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10310
+
+  if (V1HasXXSWAPD || V2HasXXSWAPD || Opcode == PPCISD::XXPERM) {
     if (isPPC64 && ValType != MVT::v2f64)
----------------
nit:
Since this guard is only around a smaller section of code I think you can take the `isPPC64` part out and make it part of this guard.
Like:
```
if (isPPC64 && (V1HasXXSWAPD || V2HasXXSWAPD || Opcode == PPCISD::XXPERM))
```
That way if we are not PPC64 we won't need to check any of the rest.



================
Comment at: llvm/test/CodeGen/PowerPC/xxperm-swap.ll:20
 ; CHECK-LE-P9-NEXT:    addis r3, r2, .LCPI0_0 at toc@ha
+; CHECK-LE-P9-NEXT:    vmr v2, v3
 ; CHECK-LE-P9-NEXT:    addi r3, r3, .LCPI0_0 at toc@l
----------------
This is the kind of extra copy that I mean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149083