[PATCH] D12908: [AArch64] Improved bitfield instruction selection.

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Geoff,

Thanks for working on this. I have some comments below.

Cheers,

James


================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1923
@@ -1921,2 +1922,3 @@
                                     SDValue &Src, int &ShiftAmount,
-                                    int &MaskWidth) {
+                                    int &MaskWidth,
+                                    bool BiggerPattern = false) {
----------------
This is called from 3 places after your patch. 2 of them explicitly set BiggerPattern.

It would make sense to remove the default from this parameter and just set it in the last callsite. This would also allow you to put the output MaskWidth at the end of the argument list.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:1964
@@ -1956,1 +1963,3 @@
   // amount.
+  if (ShlImm - ShiftAmount != 0 && !BiggerPattern)
+    return false;
----------------
The code here appears to contradict the comment - which is correct?

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2004
@@ -1994,6 +2003,3 @@
   // way to do that directely, e.g., via code matcher?)
-  SDValue OrOpd1Val = N->getOperand(1);
-  SDNode *OrOpd0 = N->getOperand(0).getNode();
-  SDNode *OrOpd1 = N->getOperand(1).getNode();
-  for (int i = 0; i < 2;
-       ++i, std::swap(OrOpd0, OrOpd1), OrOpd1Val = N->getOperand(0)) {
+  for (int i = 0; i < 4; ++i) {
+
----------------
Please use a capital for induction variables. `I` is normal.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2006
@@ +2005,3 @@
+
+    // try both operand orders with BiggerPattern=false first in case opnd1
+    // matches with BiggerPattern=false, which should lead to fewer instructions
----------------
I now understand what's going on here after some grepping and grokking, but this comment wasn't as helpful as it could have been. My understanding is:

There are situations where both `isBitfieldExtractOp(A, B, true)` and `isBitfieldExtractOp(B, A, false)` match. The latter should give better results.

Perhaps expanding a bit more in that comment would ease understanding?

Also, please capitalize first letters of sentences and end with a full stop (and spell out "operand" in full).

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2103
@@ +2102,3 @@
+
+  // Set Opc
+  EVT VT = N->getValueType(0);
----------------
This isn't a particularly useful comment.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2113
@@ +2112,3 @@
+
+  SDValue Opd0;
+  int DstLSB, Width;
----------------
"Op0" would be more in keeping with the rest of LLVM.

================
Comment at: lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:2119
@@ +2118,3 @@
+
+  unsigned ImmR = (VT.getSizeInBits() - DstLSB) % VT.getSizeInBits();
+  unsigned ImmS = Width - 1;
----------------
A comment here explaining what the immediate values are and why would be useful.


http://reviews.llvm.org/D12908