[PATCH] D30044: [ARM] Enable SMLAL[B|T] instruction selection

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:3132-3146
+    if (Subtarget->isThumb2()) {
+      SDValue Ops[] = { N->getOperand(0), N->getOperand(1),
+                        N->getOperand(2), N->getOperand(3), getAL(CurDAG, dl),
+                        CurDAG->getRegister(0, MVT::i32) };
+      ReplaceNode(N, CurDAG->getMachineNode(ARM::t2SMLAL, dl, MVT::i32,
+                                            MVT::i32, Ops));
       return;
----------------
The only functional change here appears to be limiting the first block to Thumb2, but the second block is no more correct on Thumb1 machines so I don't think it actually improves anything.


================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:3150
   }
+  case ARMISD::SMLALBB: {
+    SDValue Ops[] = { N->getOperand(0), N->getOperand(1), N->getOperand(2),
----------------
You shouldn't need C++ for this. Patterns embedded in Instructions can't produce more than one value, but by a quirk of notation instantiations of Pat can. So you should be able to write something like

    def : Pat<(smlalbb GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi),
       (SMLALBB $Rn, $Rm, $RLo, $RHi)>;

(after mapping ARMISD::SMLALBB to smlalbb of course).


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9472
+  }
+  if (auto Const = dyn_cast<ConstantSDNode>(SRA.getOperand(1))) {
+    if (Const->getZExtValue() != 31)
----------------
What if it's not a constant?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9476
+  }
+  if (SRA.getOperand(0) != Mul)
+    return SDValue();
----------------
What if it's not the same mul as before?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9481
+
+  if (isSExt32(Mul.getOperand(0)) && isSExt32(Mul.getOperand(1)))
+    Opcode = ARMISD::SMLALBB;
----------------
Don't you also need to know that the base type was i16? I don't see any checks here.


================
Comment at: lib/Target/ARM/ARMPatternHelpers.h:1
+#ifndef _LLVM_LIB_TARGET_ARM_PATTERNS_H
+#define _LLVM_LIB_TARGET_ARM_PATTERNS_H
----------------
Names beginning with a underscore and an upper-case letter are reserved in all contexts so you should drop the leading underscore.

You'll also need to add a copyright header. Though this whole file becomes irrelevant if you implement the actual selection in .td files and these functions can move to ARMISelLowering.h.


https://reviews.llvm.org/D30044