[ARM] Enable SMMLS isel

[samparker]

Summary

Patch enables selection of the smmls instruction for ARM targets, adding tests for thumb and arm.

[t.p.northover]

lib/Target/ARM/ARMISelDAGToDAG.cpp

3041–3042 ↗

(On Diff #64650)

I think you need isThumb2 for the Thumb mode instruction.

3045–3046 ↗

(On Diff #64650)

This condition could be inverted with a break to reduce indentation.

3054 ↗

(On Diff #64650)

Don't you also need to check that the correct Subc result is being used by N?

lib/Target/ARM/ARMInstrThumb2.td

2642 ↗

(On Diff #64650)

These changes don't appear to be related to supporting SMMLS. They're just a bit of refactoring as far as I can tell.

test/CodeGen/ARM/smml.ll

27–28 ↗

(On Diff #64650)

I think you should test actual dataflow here too (i.e. the registers that get used). It's very easy to mess that kind of thing up with manual C++ selection.

[samparker]

Thanks Tim.

[samparker]

Updated after Tim's comments, including: checking for thumb2, what value of subc the sube node uses, reducing indentation and improving the test.

[t.p.northover]

lib/Target/ARM/ARMISelDAGToDAG.cpp

3041 ↗

(On Diff #64837)

This still isn't quite right, I'm afraid. We only need Thumb2 if we're in Thumb mode. The check for both Thumb mode and Thumb2 is isThumb2 by the looks of it.

Could you also add an armv6-eabi line to the test to make sure it's working?

lib/Target/ARM/ARMInstrThumb2.td

2642 ↗

(On Diff #64837)

Any explanation for these changes?

[samparker]

lib/Target/ARM/ARMISelDAGToDAG.cpp

3041 ↗

(On Diff #64837)

ok, I will use that check when the opcode is being selected.

lib/Target/ARM/ARMInstrThumb2.td

2642 ↗

(On Diff #64837)

Yes, its just a bit of refactoring. Should I include it in a different patch instead?

[t.p.northover]

lib/Target/ARM/ARMInstrThumb2.td

2642 ↗

(On Diff #64837)

I think it's OK to review here (and have no objections), but a separate commit would definitely be appreciated.

[samparker]

Added armv6, thumbv6 and thumbv6t2 targets to the test.

[t.p.northover]

lib/Target/ARM/ARMISelDAGToDAG.cpp

3041–3042 ↗

(On Diff #64889)

OK, I see why this works now (there will never be an SMUL_LOHI in Thumb1 code) but I think it needs at least an assertion to make clear to a reader that we have considered thumbv6.

[samparker]

Added assertion to catch Thumb generation.

[t.p.northover]

Thanks Sam. I think this looks fine now.

llvm/trunk/lib/Target/ARM/ARMISelDAGToDAG.cpp

if (Subtarget->isThumb()) {
                         CurDAG->getRegister(0, MVT::i32),
                         CurDAG->getRegister(0, MVT::i32) };
       ReplaceNode(N, CurDAG->getMachineNode(
                          Subtarget->hasV6Ops() ? ARM::SMLAL : ARM::SMLALv5, dl,
                          MVT::i32, MVT::i32, Ops));
       return;
     }
   }
+  case ARMISD::SUBE: {
+    if (!Subtarget->hasV6Ops())
+      break;
+    // Look for a pattern to match SMMLS
+    // (sube a, (smul_loHi a, b), (subc 0, (smul_LOhi(a, b))))
+    if (N->getOperand(1).getOpcode() != ISD::SMUL_LOHI ||
+        N->getOperand(2).getOpcode() != ARMISD::SUBC)
+      break;
+
+    if (Subtarget->isThumb())
+      assert(Subtarget->hasThumb2() &&
+             "This pattern should not be generated for Thumb");
+
+    SDValue SmulLoHi = N->getOperand(1);
+    SDValue Subc = N->getOperand(2);
+    auto *Zero = dyn_cast<ConstantSDNode>(Subc.getOperand(0));
+
+    if (!Zero || Zero->getZExtValue() != 0 ||
+        Subc.getOperand(1) != SmulLoHi.getValue(0) ||
+        N->getOperand(1) != SmulLoHi.getValue(1) ||
+        N->getOperand(2) != Subc.getValue(1))
+      break;
+
+    unsigned Opc = Subtarget->isThumb2() ? ARM::t2SMMLS : ARM::SMMLS;
+    SDValue Ops[] = { SmulLoHi.getOperand(0), SmulLoHi.getOperand(1),
+                      N->getOperand(0), getAL(CurDAG, dl),
+                      CurDAG->getRegister(0, MVT::i32) };
+    ReplaceNode(N, CurDAG->getMachineNode(Opc, dl, MVT::i32, Ops));
+    return;
+  }
   case ISD::LOAD: {
     if (Subtarget->isThumb() && Subtarget->hasThumb2()) {
       if (tryT2IndexedLoad(N))
         return;
     } else if (Subtarget->isThumb()) {
       if (tryT1IndexedLoad(N))
         return;
     } else if (tryARMIndexedLoad(N))

llvm/trunk/lib/Target/ARM/ARMInstrThumb2.td

: T2FourReg<(outs rGPR:$Rd), (ins rGPR:$Rn, rGPR:$Rm, rGPR:$Ra), IIC_iMAC32,
               opc, "\t$Rd, $Rn, $Rm, $Ra", pattern>,
               Requires<[IsThumb2, HasDSP, UseMulOps]> {
   let Inst{31-27} = 0b11111;
   let Inst{26-23} = 0b0110;
   let Inst{22-20} = op22_20;
   let Inst{7-4} = op7_4;
 }
 
-def t2SMMLA : T2FourRegSMMLA<0b101, 0b0000, "smmla",
+def t2SMMLA :   T2FourRegSMMLA<0b101, 0b0000, "smmla",
                 [(set rGPR:$Rd, (add (mulhs rGPR:$Rm, rGPR:$Rn), rGPR:$Ra))]>;
-def t2SMMLAR: T2FourRegSMMLA<0b101, 0b0001, "smmlar", []>;
-def t2SMMLS: T2FourRegSMMLA<0b110, 0b0000, "smmls",
-                [(set rGPR:$Rd, (sub rGPR:$Ra, (mulhs rGPR:$Rn, rGPR:$Rm)))]>;
-def t2SMMLSR:T2FourRegSMMLA<0b110, 0b0001, "smmlsr", []>;
+def t2SMMLAR:   T2FourRegSMMLA<0b101, 0b0001, "smmlar", []>;
+def t2SMMLS:    T2FourRegSMMLA<0b110, 0b0000, "smmls", []>;
+def t2SMMLSR:   T2FourRegSMMLA<0b110, 0b0001, "smmlsr", []>;
 
 class T2ThreeRegSMUL<bits<3> op22_20, bits<2> op5_4, string opc,
                      list<dag> pattern>
   : T2ThreeReg<(outs rGPR:$Rd), (ins rGPR:$Rn, rGPR:$Rm), IIC_iMUL16, opc,
                "\t$Rd, $Rn, $Rm", pattern>,
     Requires<[IsThumb2, HasDSP]> {
     let Inst{31-27} = 0b11111;
     let Inst{26-23} = 0b0110;

llvm/trunk/test/CodeGen/ARM/smml.ll

-; RUN: llc -mtriple=arm-eabi -mcpu=cortex-a8 %s -o - | FileCheck %s
+; RUN: llc -mtriple=arm-eabi %s -o - | FileCheck %s -check-prefix=CHECK
+; RUN: llc -mtriple=armv6-eabi %s -o - | FileCheck %s -check-prefix=CHECK-V6
+; RUN: llc -mtriple=armv7-eabi %s -o - | FileCheck %s -check-prefix=CHECK-V7
+; RUN: llc -mtriple=thumb-eabi %s -o - | FileCheck %s -check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv6-eabi %s -o - | FileCheck %s -check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv6t2-eabi %s -o - | FileCheck %s -check-prefix=CHECK-THUMBV6T2
+; RUN: llc -mtriple=thumbv7-eabi %s -o - | FileCheck %s -check-prefix=CHECK-THUMBV7
 
-define i32 @f(i32 %a, i32 %b, i32 %c) nounwind readnone ssp {
+define i32 @Test0(i32 %a, i32 %b, i32 %c) nounwind readnone ssp {
 entry:
+; CHECK-LABEL: Test0
 ; CHECK-NOT: smmls
+; CHECK-V6-NOT: smmls
+; CHECK-V7-NOT: smmls
+; CHECK_THUMB-NOT: smmls
+; CHECK-THUMBV6T2-NOT: smmls
+; CHECK-THUMBV7-NOT: smmls
   %conv4 = zext i32 %a to i64
   %conv1 = sext i32 %b to i64
   %conv2 = sext i32 %c to i64
   %mul = mul nsw i64 %conv2, %conv1
   %shr5 = lshr i64 %mul, 32
   %sub = sub nsw i64 %conv4, %shr5
   %conv3 = trunc i64 %sub to i32
   ret i32 %conv3
 }
+
+define i32 @Test1(i32 %a, i32 %b, i32 %c) {
+;CHECK-LABEL: Test1
+;CHECK-NOT: smmls
+;CHECK-THUMB-NOT: smmls
+;CHECK-V6: smmls r0, [[Rn:r[1-2]]], [[Rm:r[1-2]]], r0
+;CHECK-V7: smmls r0, [[Rn:r[1-2]]], [[Rm:r[1-2]]], r0
+;CHECK-THUMBV6T2: smmls r0, [[Rn:r[1-2]]], [[Rm:r[1-2]]], r0
+;CHECK-THUMBV7: smmls r0, [[Rn:r[1-2]]], [[Rm:r[1-2]]], r0
+entry:
+  %conv = sext i32 %b to i64
+  %conv1 = sext i32 %c to i64
+  %mul = mul nsw i64 %conv1, %conv
+  %conv26 = zext i32 %a to i64
+  %shl = shl nuw i64 %conv26, 32
+  %sub = sub nsw i64 %shl, %mul
+  %shr7 = lshr i64 %sub, 32
+  %conv3 = trunc i64 %shr7 to i32
+  ret i32 %conv3
+}