[PATCH] D14380: [ARM] Combine BFIs together

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a subscriber: sbaranga.
sbaranga added a comment.

Hi James,

I had a look at this patch (see comments inline).

Cheers,
Silviu


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9046
@@ +9045,3 @@
+static bool BitsProperlyConcatenate(const APInt &A, const APInt &B) {
+  unsigned LastActiveBitInA = A.getBitWidth() - A.countTrailingZeros() - 1;
+  unsigned FirstActiveBitInB = B.countLeadingZeros();
----------------
Is this correct? Maybe I'm not correctly understanding this.

For example A = 0b0011110000 (10 bits), LastActiveBitInA is 10 - 4 - 1 = 5, which doesn't look right.

Also, what happens when either A or B is 0?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9067
@@ +9066,3 @@
+    if (NewFrom != From)
+      // The BFIs have different bases so are not compatible!
+      return SDValue();
----------------
I think you need to expand this comment.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9138
@@ +9137,3 @@
+
+    if (NewFromMask[0] == 0)
+      From1 = DCI.DAG.getNode(
----------------
Do you think it would be worth it to emit a SHR instead of bailing out here?


Repository:
  rL LLVM

http://reviews.llvm.org/D14380