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

[Eli Friedman via Phabricator via llvm-commits]

efriedma requested changes to this revision.
efriedma added a comment.
This revision now requires changes to proceed.

Please put an explanation of the .td file changes into the commit message.  (I think I follow why it's related, but it wasn't immediately obvious.)

Would it make sense to use ComputeNumSignBits to figure out whether a value is sign-extended?  That would handle, for example, cases where the 16-bit inputs are loads, or one of the 16-bit inputs is a constant.



================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9448
+
+  if (!Subtarget->hasDSP())
+    return SDValue();
----------------
The .td file says v5TE is required; this says DSP is required.  Which is correct?


================
Comment at: lib/Target/ARM/ARMPatternHelpers.cpp:7
+bool isSExt32(const SDValue &Op) {
+  return ((Op.getOpcode() == ISD::SIGN_EXTEND ||
+           Op.getOpcode() == ISD::SIGN_EXTEND_INREG ||
----------------
I know you're just moving the code, but it doesn't make sense to check for SIGN_EXTEND here.  ARM doesn't have 16-bit integer registers.


================
Comment at: test/CodeGen/ARM/longMAC.ll:243
+;CHECK-V6-THUMB2-NOT: sxth
+;CHECK-V6-THUMB2: smlalbb [[RDLO:r[0-9]+]], [[RDHI:r[0-9]+]], [[LHS:r[0-9]+]], [[RHS:r[0-9]+]]
+;CHECK-V6-THUMB2: mov r0, [[RDLO]]
----------------
The "CHECK-V6-THUMB2-NOT" aren't that useful; can you instead use CHECK-NEXT to check that there aren't any extra instructions in the function?

You might as well just use explicit register names here; given the calling convention is known, there's only one possible register assignment.


https://reviews.llvm.org/D30044