[PATCH] D19825: Power9 - Add exploitation of vector load and store that do not require swaps

[Nemanja Ivanovic via llvm-commits]

nemanjai added a comment.

In https://reviews.llvm.org/D19825#483165, @kbarton wrote:

> Sorry for another revision, but I think we need to cleanup the way different pieces of this are guarded. Plus, we should add some additional testing to cover cases with https://reviews.llvm.org/P9 but no LXVX/STXVX instructions.


I'm happy to publish another review, that's fine. Particularly because you've definitely pointed out a bug - as it is now, we'd try to use LXVX/STXVX to reload/spill even with -mcpu=pwr9 -mattr=-power9-vector which would probably result in a crash.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10633
@@ -10632,2 +10632,3 @@
     // For little endian, VSX stores require generating xxswapd/lxvd2x.
+    // Not needed on ISA 3.0 based CPUs since we have a non-permuting store.
     EVT VT = N->getOperand(1).getValueType();
----------------
kbarton wrote:
> Here the comments refer to ISA3_0. Does this imply P9Vector? What about hasVSX()?
If you don't mind, I'd like to keep the text in the comment as "ISA 3.0" because that is when the actual instructions were introduced. I prefer to keep general comments like this descriptive and the use of P9Vector is a kind of an implementation detail that isn't necessarily relevant.

Of course, if you feel strongly about it, I'll certainly change it.

================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1008
@@ -1005,2 +1007,3 @@
   } else if (PPC::VSRCRegClass.hasSubClassEq(RC)) {
-    NewMIs.push_back(addFrameReference(BuildMI(MF, DL, get(PPC::STXVD2X))
+    unsigned Op = Subtarget.isISA3_0() ? PPC::STXVX : PPC::STXVD2X;
+    NewMIs.push_back(addFrameReference(BuildMI(MF, DL, get(Op))
----------------
kbarton wrote:
> Here we check for ISA3_0. Again, does this imply P9Vector()? Are the LXVX and STXVX instructions guarded by ISA3_0 or P9Vector?
This is a good point. You're absolutely right - we need to be checking P9Vector because that is the Predicate in the .td file. I'll change this.

================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:1130
@@ -1126,2 +1129,3 @@
   } else if (PPC::VSRCRegClass.hasSubClassEq(RC)) {
-    NewMIs.push_back(addFrameReference(BuildMI(MF, DL, get(PPC::LXVD2X), DestReg),
+    unsigned Op = Subtarget.isISA3_0() ? PPC::LXVX : PPC::LXVD2X;
+    NewMIs.push_back(addFrameReference(BuildMI(MF, DL, get(Op), DestReg),
----------------
Same here. I'll change this to P9Vector.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:95
@@ -94,2 +94,3 @@
 def IsBigEndian : Predicate<"!PPCSubTarget->isLittleEndian()">;
+def HasOnlySwappingMemOps : Predicate<"!PPCSubTarget->hasP9Vector()">;
 
----------------
kbarton wrote:
> Here we're checking for P9Vector only, no ISA3_0. 
Yup. This is the right thing to do - it's the other uses that are wrong. We need to do it this way because the new instructions are guarded by P9Vector.

================
Comment at: lib/Target/PowerPC/PPCSubtarget.h:279
@@ +278,3 @@
+  bool needsSwapsForVSXMemOps() const {
+    return hasVSX() && isLittleEndian() && !isISA3_0();
+  }
----------------
kbarton wrote:
> Here we're checking hasVSX() and ISA3_0, but no mention of P9Vector. 
Yup. This will change as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D19825