[PATCH] D20239: [Power9] Add codegen for VSX word insert/extract instructions

Ehsan Amiri via llvm-commits llvm-commits at lists.llvm.org
Fri May 20 11:40:42 PDT 2016


amehsan added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1507-1508
@@ +1506,4 @@
+/// element in the desired location of the target vector.
+bool PPC::isXXINSERTWMask(ShuffleVectorSDNode *N, unsigned &Op2Shift,
+                          unsigned &Op1Byte, bool &Swap, bool IsLE) {
+
----------------
I would rename this two variables: Op2Shift and Op1Byte, in a way that are consistent with each other. Because they are named very similarly, reader thinks that they should be interpreted in the same way. Op2Shift is easy to understand (Operand 2 of Shift instruction, may be Op2OfShift?) but the other one is hard to understand from the name. This should be what we want to choose as UIM in xxinsertw. is that correct? I think it will be easier to understand if the name reflects that.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1527-1528
@@ +1526,4 @@
+  unsigned M3 = N->getMaskElt(12) / 4;
+  unsigned LeShifts[] = { 2, 1, 0, 3 };
+  unsigned BeShifts[] = { 3, 0, 1, 2 };
+
----------------
A comment here to say what le and be means, or changing the name to LittleEndinanShifts and BigEndianShifts, could help people who do not have enough context about this, if they happen to look at the code.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1530-1531
@@ +1529,4 @@
+
+  // Below H is in [4,7] and L is in [0,3]
+  // H, 1, 2, 3 or L, 5, 6, 7
+  if ((M0 > 3 && M1 == 1 && M2 == 2 && M3 == 3) ||
----------------
This is a cryptic comment. What is H, what is L? 

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1532-1562
@@ +1531,33 @@
+  // H, 1, 2, 3 or L, 5, 6, 7
+  if ((M0 > 3 && M1 == 1 && M2 == 2 && M3 == 3) ||
+      (M0 < 4 && M1 == 5 && M2 == 6 && M3 == 7)) {
+    Op2Shift = IsLE ? LeShifts[M0 & 0x3] : BeShifts[M0 & 0x3];
+    Op1Byte = IsLE ? 12 : 0;
+    Swap = M0 < 4;
+    return true;
+  }
+  // 0, H, 2, 3 or 4, L, 6, 7
+  if ((M1 > 3 && M0 == 0 && M2 == 2 && M3 == 3) ||
+      (M1 < 4 && M0 == 4 && M2 == 6 && M3 == 7)) {
+    Op2Shift = IsLE ? LeShifts[M1 & 0x3] : BeShifts[M1 & 0x3];
+    Op1Byte = IsLE ? 8 : 4;
+    Swap = M1 < 4;
+    return true;
+  }
+  // 0, 1, H, 3 or 4, 5, L, 7
+  if ((M2 > 3 && M0 == 0 && M1 == 1 && M3 == 3) ||
+      (M2 < 4 && M0 == 4 && M1 == 5 && M3 == 7)) {
+    Op2Shift = IsLE ? LeShifts[M2 & 0x3] : BeShifts[M2 & 0x3];
+    Op1Byte = IsLE ? 4 : 8;
+    Swap = M2 < 4;
+    return true;
+  }
+  // 0, 1, 2, H or 4, 5, 6, L
+  if ((M3 > 3 && M0 == 0 && M1 == 1 && M2 == 2) ||
+      (M3 < 4 && M0 == 4 && M1 == 5 && M2 == 6)) {
+    Op2Shift = IsLE ? LeShifts[M3 & 0x3] : BeShifts[M3 & 0x3];
+    Op1Byte = IsLE ? 0 : 12;
+    Swap = M3 < 4;
+    return true;
+  }
+  return false;
----------------
I think if the two vector operands of shuffle vector are the same, then you can allow some other combinations of M1 and M2.


Repository:
  rL LLVM

http://reviews.llvm.org/D20239





More information about the llvm-commits mailing list