ARM SMLAW[B|T], SMULW[B|T] ISel

[samparker]

Summary

Added ISel for smlawb, smlawt, smulwb and smulwt instructions for the ARM backend, along with CodeGen tests.
A fix for the bug reported in llvm.org/bugs/show_bug.cgi?id=21297

[rengolin]

Hi Sam,

The patch looks good. My comments are more about the format than the contents, which seems ok.

I don't particularly like how the Opc is set down all those nested functions across the multiple branches, but I don't have a better idea. This sequence of instructions is really hard to map into a simple structure (too many orthogonal variations).

Can you re-format and re-submit with -U999 so we can have a bit more context, please?

cheers,
--renato

lib/Target/ARM/ARMISelDAGToDAG.cpp

94

irrelevant whispace change

2534

clang format

test/CodeGen/ARM/smul.ll

39

better to use

CHECK-LABEL: @f4

on all of them

[samparker]

same diff, just with added context

[samparker]

Hi Renato,

Thanks for the review and sorry I forgot about the added context again, I will add the changes after further review.

cheers,
sam

[rengolin]

lib/Target/ARM/ARMISelDAGToDAG.cpp

254

It seems these two could be coalesced into one method with a bit more logic inside the method... They're not really that different and they both call the same private static methods anyway.

2596

Maybe you should also test the opcode here and return null?

[samparker]

Combined the SelectSMLAWX and SelectSMULWX functions into one, as well as adding extra checks that Opc gets assigned before creating the new node. Also updated the test check labels.

[rengolin]

LGTM. Thanks!

lib/Target/ARM/ARMISelDAGToDAG.cpp

   /// getI32Imm - Return a target constant of type i32 with the specified
   /// value.
   inline SDValue getI32Imm(unsigned Imm, SDLoc dl) {
     return CurDAG->getTargetConstant(Imm, dl, MVT::i32);
   }
 
   SDNode *Select(SDNode *N) override;
 
-
   bool hasNoVMLxHazardUse(SDNode *N) const;
   bool isShifterOpProfitable(const SDValue &Shift,
                              ARM_AM::ShiftOpc ShOpcVal, unsigned ShAmt);
   bool SelectRegShifterOperand(SDValue N, SDValue &A,
                                SDValue &B, SDValue &C,
                                bool CheckProfitability = true);
   bool SelectImmShifterOperand(SDValue N, SDValue &A,
                                SDValue &B, bool CheckProfitability = true);
   bool SelectShiftRegShifterOperand(SDValue N, SDValue &A,

 
   SDNode *SelectReadRegister(SDNode *N);
   SDNode *SelectWriteRegister(SDNode *N);
 
   SDNode *SelectInlineAsm(SDNode *N);
 
   SDNode *SelectConcatVector(SDNode *N);
 
+  SDNode *SelectSMLAWX(SDNode *N);
+
+  SDNode *SelectSMULWX(SDNode *N);
+
   /// SelectInlineAsmMemoryOperand - Implement addressing mode selection for
   /// inline asm expressions.
   bool SelectInlineAsmMemoryOperand(const SDValue &Op, unsigned ConstraintID,
                                     std::vector<SDValue> &OutOps) override;
 
   // Form pairs of consecutive R, S, D, or Q registers.
   SDNode *createGPRPairNode(EVT VT, SDValue V0, SDValue V1);
   SDNode *createSRegPairNode(EVT VT, SDValue V0, SDValue V1);

       Size == SRAConstant->getZExtValue()) {
     unsigned Opcode = Subtarget->isThumb2() ? ARM::t2ABS : ARM::ABS;
     return CurDAG->SelectNodeTo(N, Opcode, VT, ADDSrc0);
   }
 
   return nullptr;
 }
 
+static bool SearchSignedMulShort(SDValue SignExt, unsigned *Opc, SDValue &Src1,
+                                 bool Accumulate) {
+  // For SM*WB, we need to some form of sext.
+  // For SM*WT, we need to search for (sra X, 16)
+  // Src1 then gets set to X.
+  if ((SignExt.getOpcode() == ISD::SIGN_EXTEND ||
+       SignExt.getOpcode() == ISD::SIGN_EXTEND_INREG ||
+       SignExt.getOpcode() == ISD::AssertSext) &&
+       SignExt.getValueType() == MVT::i32) {
+
+    *Opc = Accumulate ? ARM::SMLAWB : ARM::SMULWB;
+    Src1 = SignExt.getOperand(0);
+    return true;
+  }
+
+  if (SignExt.getOpcode() != ISD::SRA)
+    return false;
+
+  ConstantSDNode *SRASrc1 = dyn_cast<ConstantSDNode>(SignExt.getOperand(1));
+  if (!SRASrc1 || SRASrc1->getZExtValue() != 16)
+    return false;
+
+  SDValue Op0 = SignExt.getOperand(0);
+
+  // The sign extend operand for SM*WB could be generated by a shl and ashr.
+  if (Op0.getOpcode() == ISD::SHL) {
+    SDValue SHL = Op0;
+    ConstantSDNode *SHLSrc1 = dyn_cast<ConstantSDNode>(SHL.getOperand(1));
+    if (!SHLSrc1 || SHLSrc1->getZExtValue() != 16)
+      return false;
+
+    *Opc = Accumulate ? ARM::SMLAWB : ARM::SMULWB;
+    Src1 = Op0.getOperand(0);
+    return true;
+  }
+  *Opc = Accumulate ? ARM::SMLAWT : ARM::SMULWT;
+  Src1 = SignExt.getOperand(0);
+  return true;
+}
+
+static bool SearchSignedMulLong(SDValue OR, unsigned *Opc, SDValue &Src0,
+                                SDValue &Src1, bool Accumulate) {
+  // First we look for:
+  // (add (or (srl ?, 16), (shl ?, 16)))
+  if (OR.getOpcode() != ISD::OR)
+    return false;
+
+  SDValue SRL = OR.getOperand(0);
+  SDValue SHL = OR.getOperand(1);
+
+  if (SRL.getOpcode() != ISD::SRL ||
+      SHL.getOpcode() != ISD::SHL) {
+    SRL = OR.getOperand(1);
+    SHL = OR.getOperand(0);
+    if (SRL.getOpcode() != ISD::SRL ||
+        SHL.getOpcode() != ISD::SHL)
+      return false;
+  }
+
+  ConstantSDNode *SRLSrc1 = dyn_cast<ConstantSDNode>(SRL.getOperand(1));
+  ConstantSDNode *SHLSrc1 = dyn_cast<ConstantSDNode>(SHL.getOperand(1));
+  if (!SRLSrc1 ||
+      !SHLSrc1 ||
+      SRLSrc1->getZExtValue() != 16 ||
+      SHLSrc1->getZExtValue() != 16)
+    return false;
+
+  // The first operands to the shifts need to be the two results from the
+  // same smul_lohi node.
+  if ((SRL.getOperand(0).getNode() != SHL.getOperand(0).getNode()) ||
+       SRL.getOperand(0).getOpcode() != ISD::SMUL_LOHI)
+    return false;
+
+  SDNode *SMULLOHI = SRL.getOperand(0).getNode();
+  if (SRL.getOperand(0) != SDValue(SMULLOHI, 0) ||
+      SHL.getOperand(0) != SDValue(SMULLOHI, 1))
+    return false;
+
+  // Now we have:
+  // (add (or (srl (smul_lohi ?, ?), 16), (shl (smul_lohi ?, ?), 16)))
+  // For SMLAW[B|T] smul_lohi will take a 32-bit and a 16-bit arguments.
+  // For SMLAWB the 16-bit value will signed extended somehow.
+  // For SMLAWT only the SRA is required.
+
+  // Check both sides of SMUL_LOHI
+  if (SearchSignedMulShort(SMULLOHI->getOperand(0), Opc, Src1, Accumulate)) {
+    Src0 = SMULLOHI->getOperand(1);
+  } else if (SearchSignedMulShort(SMULLOHI->getOperand(1), Opc, Src1,
+                                  Accumulate)) {
+    Src0 = SMULLOHI->getOperand(0);
+  } else {
+    return false;
+  }
+  return true;
+}
+
+SDNode *ARMDAGToDAGISel::SelectSMLAWX(SDNode *N) {
+  SDLoc dl(N);
+  SDValue Src0 = N->getOperand(0);
+  SDValue Src1 = N->getOperand(1);
+
+  if (Src0.getOpcode() != ISD::OR &&
+      Src1.getOpcode() != ISD::OR)
+    return nullptr;
+
+  SDValue A, B, Acc;
+  unsigned Opc;
+  if (SearchSignedMulLong(Src0, &Opc, A, B, true)) {
+    Acc = Src1;
+  } else if (SearchSignedMulLong(Src1, &Opc, A, B, true)) {
+    Acc = Src0;
+  } else {
+    return nullptr;
+  }
+  SDValue Ops[] = { A, B, Acc,
+                    getAL(CurDAG, dl), CurDAG->getRegister(0, MVT::i32),
+                    CurDAG->getEntryNode() };
+  return CurDAG->SelectNodeTo(N, Opc, MVT::i32, MVT::Other, Ops);
+}
+
+SDNode *ARMDAGToDAGISel::SelectSMULWX(SDNode *N) {
+  SDLoc dl(N);
+  SDValue A, B;
+  unsigned Opc;
+
+  if (SearchSignedMulLong(SDValue(N, 0), &Opc, A, B, false)) {
+    SDValue Ops[] = { A, B,
+                      getAL(CurDAG, dl), CurDAG->getRegister(0, MVT::i32)};
+    return CurDAG->SelectNodeTo(N, Opc, MVT::i32, Ops);
+  }
+  return nullptr;
+}
+
 SDNode *ARMDAGToDAGISel::SelectConcatVector(SDNode *N) {
   // The only time a CONCAT_VECTORS operation can have legal types is when
   // two 64-bit vectors are concatenated to a 128-bit vector.
   EVT VT = N->getValueType(0);
   if (!VT.is128BitVector() || N->getNumOperands() != 2)
     llvm_unreachable("unexpected CONCAT_VECTORS");
   return createDRegPairNode(VT, N->getOperand(0), N->getOperand(1));
 }
 
 SDNode *ARMDAGToDAGISel::Select(SDNode *N) {
   SDLoc dl(N);
 
   if (N->isMachineOpcode()) {
     N->setNodeId(-1);
     return nullptr;   // Already selected.
   }
 
   switch (N->getOpcode()) {
   default: break;
+  case ISD::ADD: {
+    SDNode *ResNode = SelectSMLAWX(N);
+    if (ResNode)
+      return ResNode;
+    break;
+  }
+  case ISD::OR: {
+    SDNode *ResNode = SelectSMULWX(N);
+    if (ResNode)
+      return ResNode;
+    break;
+  }
   case ISD::WRITE_REGISTER: {
     SDNode *ResNode = SelectWriteRegister(N);
     if (ResNode)
       return ResNode;
     break;
   }
   case ISD::READ_REGISTER: {
     SDNode *ResNode = SelectReadRegister(N);

test/CodeGen/ARM/smul.ll

 ; RUN: llc -mtriple=arm-eabi -mcpu=generic %s -o /dev/null
 ; RUN: llc -mtriple=arm-eabi -mcpu=cortex-a8 %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumb--none-eabi -mcpu=cortex-a8 %s -o - | FileCheck %s
 
 @x = weak global i16 0          ; <i16*> [#uses=1]
 @y = weak global i16 0          ; <i16*> [#uses=0]
 
 define i32 @f1(i32 %y) {
 ; CHECK: f1
 ; CHECK: smulbt
         %tmp = load i16, i16* @x             ; <i16> [#uses=1]

 ; CHECK: smlabt
         %tmp = sext i16 %x to i32               ; <i32> [#uses=1]
         %tmp2 = ashr i32 %y, 16         ; <i32> [#uses=1]
         %tmp3 = mul i32 %tmp2, %tmp             ; <i32> [#uses=1]
         %tmp5 = add i32 %tmp3, %a               ; <i32> [#uses=1]
         ret i32 %tmp5
 }
 
+define i32 @f4(i32 %a, i32 %x, i32 %y) {
+; CHECK: f4
+; CHECK: smlatt
+        %tmp1 = ashr i32 %x, 16
+        %tmp3 = ashr i32 %y, 16
+        %tmp4 = mul i32 %tmp3, %tmp1
+        %tmp5 = add i32 %tmp4, %a
+        ret i32 %tmp5
+}
+
+define i32 @f5(i32 %a, i16 %x, i16 %y) {
+; CHECK: f5
+; CHECK: smlabb
+        %tmp1 = sext i16 %x to i32
+        %tmp3 = sext i16 %y to i32
+        %tmp4 = mul i32 %tmp3, %tmp1
+        %tmp5 = add i32 %tmp4, %a
+        ret i32 %tmp5
+}
+
+define i32 @f6(i32 %a, i16 %x, i32 %y) {
+; CHECK: f6
+; CHECK: smlabt
+        %tmp1 = sext i16 %x to i32
+        %tmp3 = ashr i32 %y, 16
+        %tmp4 = mul i32 %tmp3, %tmp1
+        %tmp5 = add i32 %tmp4, %a
+        ret i32 %tmp5
+}
+
+define i32 @f7(i32 %a, i32 %b, i32 %c) {
+; CHECK: f7
+; CHECK: smlawb
+        %shl = shl i32 %b, 16
+        %shr = ashr exact i32 %shl, 16
+        %conv = sext i32 %a to i64
+        %conv2 = sext i32 %shr to i64
+        %mul = mul nsw i64 %conv2, %conv
+        %shr49 = lshr i64 %mul, 16
+        %conv5 = trunc i64 %shr49 to i32
+        %add = add nsw i32 %conv5, %c
+        ret i32 %add
+}
+
+define i32 @f8(i32 %a, i16 signext %b, i32 %c) {
+; CHECK: f8
+; CHECK: smlawb
+        %conv = sext i32 %a to i64
+        %conv1 = sext i16 %b to i64
+        %mul = mul nsw i64 %conv1, %conv
+        %shr5 = lshr i64 %mul, 16
+        %conv2 = trunc i64 %shr5 to i32
+        %add = add nsw i32 %conv2, %c
+        ret i32 %add
+}
+
+define i32 @f9(i32 %a, i32 %b, i32 %c) {
+; CHECK: f9
+; CHECK: smlawt
+        %conv = sext i32 %a to i64
+        %shr = ashr i32 %b, 16
+        %conv1 = sext i32 %shr to i64
+        %mul = mul nsw i64 %conv1, %conv
+        %shr26 = lshr i64 %mul, 16
+        %conv3 = trunc i64 %shr26 to i32
+        %add = add nsw i32 %conv3, %c
+        ret i32 %add
+}
+
+define i32 @f10(i32 %a, i32 %b, i32 %c) {
+; CHECK: f10
+; CHECK: smulwb
+        %shl = shl i32 %b, 16
+        %shr = ashr exact i32 %shl, 16
+        %conv = sext i32 %a to i64
+        %conv2 = sext i32 %shr to i64
+        %mul = mul nsw i64 %conv2, %conv
+        %shr37 = lshr i64 %mul, 16
+        %conv4 = trunc i64 %shr37 to i32
+        ret i32 %conv4
+}
+
+define i32 @f11(i32 %a, i16 signext %b, i32 %c) {
+; CHECK: f11
+; CHECK: smulwb
+        %conv = sext i32 %a to i64
+        %conv1 = sext i16 %b to i64
+        %mul = mul nsw i64 %conv1, %conv
+        %shr4 = lshr i64 %mul, 16
+        %conv2 = trunc i64 %shr4 to i32
+        ret i32 %conv2
+}
+
+define i32 @f12(i32 %a, i32 %b, i32 %c) {
+; CHECK: f12
+; CHECK: smulwt
+        %conv = sext i32 %a to i64
+        %shr = ashr i32 %b, 16
+        %conv1 = sext i32 %shr to i64
+        %mul = mul nsw i64 %conv1, %conv
+        %shr25 = lshr i64 %mul, 16
+        %conv3 = trunc i64 %shr25 to i32
+        ret i32 %conv3
+}

test/CodeGen/ARM/smulw.ll

-; RUN: llc -mtriple=arm--none-eabi -mcpu=cortex-a8 %s -o - | FileCheck %s
-; RUN: llc -mtriple=thumb--none-eabi -mcpu=cortex-a8 %s -o - | FileCheck %s
-
-; We cannot codegen the smulw[bt] or smlaw[bt] instructions for these functions,
-; as the top 16 bits of the result would differ
-
-define i32 @f1(i32 %a, i16 %b) {
-; CHECK-LABEL: f1:
-; CHECK: mul
-; CHECK: asr
-  %tmp1 = sext i16 %b to i32
-  %tmp2 = mul i32 %a, %tmp1
-  %tmp3 = ashr i32 %tmp2, 16
-  ret i32 %tmp3
-}
-
-define i32 @f2(i32 %a, i16 %b, i32 %c) {
-; CHECK-LABEL: f2:
-; CHECK: mul
-; CHECK: add{{.*}}, asr #16
-  %tmp1 = sext i16 %b to i32
-  %tmp2 = mul i32 %a, %tmp1
-  %tmp3 = ashr i32 %tmp2, 16
-  %tmp4 = add i32 %tmp3, %c
-  ret i32 %tmp4
-}