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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/Target/ARM/ARMInstrInfo.td:4202
+
+def : Pat<(ARMsmlalbb GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi),
+          (SMLALBB $Rn, $Rm, $RLo, $RHi)>;
----------------
t.p.northover wrote:
> I think these should be ARMPat or they'll still be valid in Thumb mode. You'll also need Thumb patterns to select that variant.
cheers!


================
Comment at: lib/Target/ARM/ARMPatternHelpers.cpp:45-60
+bool isBottomS16(const SDValue &Op, SelectionDAG *CurDAG) {
+  if (isSRA16(Op))
+    return isSHL16(Op.getOperand(0));
+
+  return CurDAG->ComputeNumSignBits(Op) == 17;
+}
+
----------------
t.p.northover wrote:
> This combination looks extremely dodgy to me.  ComputeNumSignBits makes no promises about how it's going to find those bits, and in fact traverses nodes down to a depth of 6.
> 
> You've already found two cases where the final SDNode's operand(0) isn't actually the unextended node, I see no reason to think that's all there is.
Ah yes... Do you think it's worth adding a function to SelectionDAG that returns the SDValue that ComputeNumSignBits finds and uses for its calculation?


https://reviews.llvm.org/D30044