[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/ARMISelLowering.cpp:9476
+  }
+  if (SRA.getOperand(0) != Mul)
+    return SDValue();
----------------
samparker wrote:
> t.p.northover wrote:
> > samparker wrote:
> > > t.p.northover wrote:
> > > > What if it's not the same mul as before?
> > > Sorry I don't get your point, what I am missing?
> > Just knowing that the input to the SRA is **some** multiply operation isn't sufficient. You need to make sure it's the same one that produced the low bits.
> Is this not what I have done? I'm checking that the MUL operand of the ADDC is the same operand to the SRA.
Bother, sorry I completely misread it.


================
Comment at: lib/Target/ARM/ARMInstrInfo.td:4202
+
+def : Pat<(ARMsmlalbb GPR:$Rn, GPR:$Rm, GPR:$RLo, GPR:$RHi),
+          (SMLALBB $Rn, $Rm, $RLo, $RHi)>;
----------------
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.


================
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;
+}
+
----------------
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.


https://reviews.llvm.org/D30044