[PATCH] [PPC64LE] Remove unnecessary swaps from lane-insensitive vector computations

[hfinkel at anl.gov]

I apologize for taking so long to get to this...


REPOSITORY
  rL LLVM

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:21
@@ +20,3 @@
+// The extra xxswapd instructions reduce performance.  This can be
+// particularly bad for vectorized code.  The purpose of this pass
+// is to reduce the number of xxswapd instructions required for
----------------
What does "particularly bad for vectorized code" mean? Doesn't this exclusively apply to vectorized code?


================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:126
@@ +125,3 @@
+  // Walk the machine instructions to gather vector usage information.
+  // Return true iff vector mentions are present.
+  bool gatherVectorMentions();
----------------
Why don't we just call these "vector instructions"? mentions sounds stilted to me.

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:167
@@ +166,3 @@
+  // Return true iff the given register is a full vector register.
+  bool isVecReg(unsigned Reg) {
+    return (isRegInClass(Reg, &PPC::VSRCRegClass) ||
----------------
As a general note, this pass does not handle the case where a register has a subregister index. This should be mostly irrelevant here (we probably don't generate them for these kinds of instructions), but we also don't want to mishandle them should they occur.

(maybe also give up in lookThruCopyLike should you run into a register with a subreg index too).

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:224
@@ +223,3 @@
+
+  for (MachineFunction::iterator I = MF->begin(); I != MF->end();) {
+    MachineBasicBlock &MBB = *I++;
----------------
range-based for?

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:227
@@ +226,3 @@
+
+    for (MachineBasicBlock::iterator BI = MBB.begin(), BIE = MBB.end();
+         BI != BIE; ++BI) {
----------------
range-based for?

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:232
@@ +231,3 @@
+
+      for (unsigned Idx = 0, Last = MI->getNumOperands(); Idx != Last; ++Idx) {
+        MachineOperand &MO = MI->getOperand(Idx);
----------------
  for (const MachineOperand &MO : MI->operands()) {

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:284
@@ +283,3 @@
+        // handling for this in the future.
+        SwapVector[VecIdx].IsLoad = 1;
+        break;
----------------
Where to we actually reject webs with non-swapping load/stores?


================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:307
@@ +306,3 @@
+        // These are fine provided they are moving between full vector
+        // register classes.
+        if (isVecReg(MI->getOperand(0).getReg()) &&
----------------
We should add a comment here explaining that this happens because some of the 128-bit VSX registers have 128-bit Altivec sub-registers.

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:333
@@ +332,3 @@
+      // FIXME: Is there a way we could specify this as a flag on the
+      // instruction in the tblgen description?  That would seem
+      // a better approach for future maintenance, but I don't
----------------
Yes, that would be better. If nothing else, we should be able to use the InstrMapping facility to generate a lookup table (this is how we currently keep track of which instructions are record forms, and the mapping between record-form and the non-record-form variants).


================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:459
@@ +458,3 @@
+    CopySrcReg = MI->getOperand(1).getReg();
+  else // isSubregToReg()
+    CopySrcReg = MI->getOperand(2).getReg();
----------------
add an assert that isSubregToReg() is true.

================
Comment at: lib/Target/PowerPC/PPCVSXSwapRemoval.cpp:491
@@ +490,3 @@
+    // for physical regs.
+    for (unsigned Idx = 0, Last = MI->getNumOperands(); Idx != Last; ++Idx) {
+
----------------
range-based for?

http://reviews.llvm.org/D8565

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/