[PATCH] D12196: [ARM] Extract shifts out of multiply-by-constant

[Renato Golin via llvm-commits]

rengolin added a comment.

Hi John,

Sorry for the delay, I was on holidays.

cheers,
-renato


================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:523
@@ +522,3 @@
+  // Replace the constant with one with the shift extracted
+  SDValue NewConstant = CurDAG->getConstant(NewMulConstVal, SDLoc(N), MVT::i32);
+  CurDAG->RepositionNode(N.getNode(), NewConstant.getNode());
----------------
I think this should just return true/false, and let the caller change the constant at the same time as applying the shift.

My worry is if you use this in an if() with other statements, like:

    if (ExtractShiftFromMul() && SomeOtherTest())
      return false;

and "SomeOtherTest()" fails, returning with the changed constant but not applying the shift.

================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:538
@@ +537,3 @@
+  // use it in a shifted operand do so.
+  if (N.getOpcode() == ISD::MUL && N.hasOneUse()) {
+    unsigned PowerOfTwo = 0;
----------------
You already test this in ExtractShiftFromMul, no?

================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:540
@@ +539,3 @@
+    unsigned PowerOfTwo = 0;
+    if (ExtractShiftFromMul(N, 31, PowerOfTwo)) {
+      BaseReg = N;
----------------
If my comment above is pertinent, maybe this should be called "canExtract..." and also change N, not only PowerOfTwo, so this block remains the same, but it's more future proof when someone adds another condition after the call to "canExtract...".

================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:735
@@ +734,3 @@
+  // and use it in a shifted operand do so.
+  if (Offset.getOpcode() == ISD::MUL && Offset.hasOneUse()) {
+    unsigned PowerOfTwo = 0;
----------------
Same comments as above.

================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:1404
@@ +1403,3 @@
+  // and use it in a shifted operand do so.
+  if (OffReg.getOpcode() == ISD::MUL && OffReg.hasOneUse()) {
+    unsigned PowerOfTwo = 0;
----------------
... :)


Repository:
  rL LLVM

http://reviews.llvm.org/D12196