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

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

In https://reviews.llvm.org/D30044#678987, @efriedma wrote:

> 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.


This operations only act upon registers, but I will definitely look into this and the load issue, thanks.



================
Comment at: lib/Target/ARM/ARMISelDAGToDAG.cpp:3150
   }
+  case ARMISD::SMLALBB: {
+    SDValue Ops[] = { N->getOperand(0), N->getOperand(1), N->getOperand(2),
----------------
t.p.northover wrote:
> 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).
Ah excellent, I will clean all this up.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9448
+
+  if (!Subtarget->hasDSP())
+    return SDValue();
----------------
efriedma wrote:
> The .td file says v5TE is required; this says DSP is required.  Which is correct?
Yes good point, hasDSP is only the Thumb check.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:9476
+  }
+  if (SRA.getOperand(0) != Mul)
+    return SDValue();
----------------
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?


================
Comment at: lib/Target/ARM/ARMPatternHelpers.h:1
+#ifndef _LLVM_LIB_TARGET_ARM_PATTERNS_H
+#define _LLVM_LIB_TARGET_ARM_PATTERNS_H
----------------
t.p.northover wrote:
> 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.
Ok. I'd still want to keep the helpers because they're used for pattern matching of different instructions in DAGToDAG as well.


================
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]]
----------------
efriedma wrote:
> 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.
I will change to explicit registers names, but I would like to keep the extra NOT checks because it caught me when I had originally lowered the operands incorrectly, which left the sxth and shifts in.


https://reviews.llvm.org/D30044