[ARM] MRRC2 shouldn't allow predicates

[rs]

Summary

The MRRC2 instruction can't be predicated, neither can the thumb variants of the instruction but the tablegen descriptions of the instructions in ARMInstrThumb2.td is wrong and will be looked at in a future patch. This patch fixes an assertion failure that picked up this problem caused by patch http://reviews.llvm.org/D21178

[rengolin]

Hi Ranjeet,

I'm surprised you didn't pick this problem before commit. Our buildbots seem happy, so this was definitely a downstream test.

Both my AArch32 and ARMv7M manuals seem to imply AL is allowed for MRRC2, but the ARMv7A manual says it must be unconditional (which following certain interpretations, could mean using AL).

Can you share what the assertion is?

cheers,
--renato

[rs]

I'm surprised you didn't pick this problem before commit. Our buildbots seem happy, so this was definitely a downstream test.

It definitely broke upstream buildbots I got the emails so reverted the patches straight away. It was due to a silly mistake of not building with asserts enabled.

Can you share what the assertion is?

The assertion happened in ./lib/CodeGen/SelectionDAG/InstrEmitter.cpp line

assert(NumMIOperands >= II.getNumOperands() &&
       NumMIOperands <= II.getNumOperands() + II.getNumImplicitDefs() +
                        NumImpUses &&
       "#operands for dag node doesn't match .td file!");

Basically it was complaining that the MRRC2 instruction that was being built in ARMISelDAGToDAG.cpp had more operands than were allowed by the tablegen description for the instruction

class MovRRCopro2<string opc, bit direction, dag oops, dag iops,
                list<dag> pattern = []>
: ABXI<0b1100, oops, iops, NoItinerary,
       !strconcat(opc, "\t$cop, $opc1, $Rt, $Rt2, $CRm"), pattern>,
  Requires<[PreV8]> {
let Inst{31-28} = 0b1111;
let Inst{23-21} = 0b010;
let Inst{20} = direction;

bits<4> Rt;
bits<4> Rt2;
bits<4> cop;
bits<4> opc1;
bits<4> CRm;

let Inst{15-12} = Rt;
let Inst{19-16} = Rt2;
let Inst{11-8}  = cop;
let Inst{7-4}   = opc1;
let Inst{3-0}   = CRm;

let DecoderMethod = "DecoderForMRRC2AndMCRR2";
}
def MRRC2 : MovRRCopro2<"mrrc2", 1 /* from coprocessor to ARM core register */,
                     (outs GPRnopc:$Rt, GPRnopc:$Rt2),
                     (ins p_imm:$cop, imm0_15:$opc1, c_imm:$CRm), []>;

As you can see the instruction doesn't allow you to specify any predicates.

Both my AArch32 and ARMv7M manuals seem to imply AL is allowed for MRRC2, but the ARMv7A manual says it must be unconditional (which following certain interpretations, could mean using AL).

Yes AL is allowed as a predicate to MRRC2 in assembly but whether it's specified or not the top 4 bits will always be 1111 not 1110 which is the encoding for AL. I've updated my patch comment as the previous comment is wrong.

Thanks,
Ranjeet

[rengolin]

In D21408#459903, @rs wrote:

It definitely broke upstream buildbots I got the emails so reverted the patches straight away. It was due to a silly mistake of not building with asserts enabled.

Right, so probably never hit our bots. I didn't see your revert, sorry.

Basically it was complaining that the MRRC2 instruction that was being built in ARMISelDAGToDAG.cpp had more operands than were allowed by the tablegen description for the instruction

(...)
As you can see the instruction doesn't allow you to specify any predicates.

Right.

Yes AL is allowed as a predicate to MRRC2 in assembly but whether it's specified or not the top 4 bits will always be 1111 not 1110 which is the encoding for AL. I've updated my patch comment as the previous comment is wrong.

Ok, it makes sense now, thanks!

Please, create a bug for addressing the FIXME, feel free to copy me.

LGTM, thanks!

llvm/trunk/include/llvm/IR/IntrinsicsARM.td

 def int_arm_cdp : GCCBuiltin<"__builtin_arm_cdp">,
    Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
                   llvm_i32_ty, llvm_i32_ty, llvm_i32_ty], []>;
 def int_arm_cdp2 : GCCBuiltin<"__builtin_arm_cdp2">,
    Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
                   llvm_i32_ty, llvm_i32_ty, llvm_i32_ty], []>;
 
 // Move from two registers to coprocessor
-def int_arm_mcrr : GCCBuiltin<"__builtin_arm_mcrr">,
-   Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
+def int_arm_mcrr : Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
                                   llvm_i32_ty, llvm_i32_ty], []>;
-def int_arm_mcrr2 : GCCBuiltin<"__builtin_arm_mcrr2">,
-   Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
+def int_arm_mcrr2 : Intrinsic<[], [llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
+                                   llvm_i32_ty, llvm_i32_ty], []>;
+
+def int_arm_mrrc : Intrinsic<[llvm_i32_ty, llvm_i32_ty], [llvm_i32_ty,
+                              llvm_i32_ty, llvm_i32_ty], []>;
+def int_arm_mrrc2 : Intrinsic<[llvm_i32_ty, llvm_i32_ty], [llvm_i32_ty,
                                llvm_i32_ty, llvm_i32_ty], []>;
 
 //===----------------------------------------------------------------------===//
 // CRC32
 
 def int_arm_crc32b  : Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty],
     [IntrNoMem]>;
 def int_arm_crc32cb : Intrinsic<[llvm_i32_ty], [llvm_i32_ty, llvm_i32_ty],
     [IntrNoMem]>;

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

void ARMDAGToDAGISel::Select(SDNode *N) {
 
   case ISD::INTRINSIC_VOID:
   case ISD::INTRINSIC_W_CHAIN: {
     unsigned IntNo = cast<ConstantSDNode>(N->getOperand(1))->getZExtValue();
     switch (IntNo) {
     default:
       break;
 
+    case Intrinsic::arm_mrrc:
+    case Intrinsic::arm_mrrc2: {
+      SDLoc dl(N);
+      SDValue Chain = N->getOperand(0);
+      unsigned Opc;
+
+      if (Subtarget->isThumb())
+        Opc = (IntNo == Intrinsic::arm_mrrc ? ARM::t2MRRC : ARM::t2MRRC2);
+      else
+        Opc = (IntNo == Intrinsic::arm_mrrc ? ARM::MRRC : ARM::MRRC2);
+
+      SmallVector<SDValue, 5> Ops;
+      Ops.push_back(getI32Imm(cast<ConstantSDNode>(N->getOperand(2))->getZExtValue(), dl)); /* coproc */
+      Ops.push_back(getI32Imm(cast<ConstantSDNode>(N->getOperand(3))->getZExtValue(), dl)); /* opc */
+      Ops.push_back(getI32Imm(cast<ConstantSDNode>(N->getOperand(4))->getZExtValue(), dl)); /* CRm */
+
+      // The mrrc2 instruction in ARM doesn't allow predicates, the top 4 bits of the encoded
+      // instruction will always be '1111' but it is possible in assembly language to specify
+      // AL as a predicate to mrrc2 but it doesn't make any difference to the encoded instruction.
+      if (Opc != ARM::MRRC2) {
+        Ops.push_back(getAL(CurDAG, dl));
+        Ops.push_back(CurDAG->getRegister(0, MVT::i32));
+      }
+
+      Ops.push_back(Chain);
+
+      // Writes to two registers.
+      std::vector<EVT> RetType;
+      RetType.push_back(MVT::i32);
+      RetType.push_back(MVT::i32);
+      RetType.push_back(MVT::Other);
+
+      ReplaceNode(N, CurDAG->getMachineNode(Opc, dl, RetType, Ops));
+      return;
+    }
     case Intrinsic::arm_ldaexd:
     case Intrinsic::arm_ldrexd: {
       SDLoc dl(N);
       SDValue Chain = N->getOperand(0);
       SDValue MemAddr = N->getOperand(2);
       bool isThumb = Subtarget->isThumb() && Subtarget->hasV8MBaselineOps();
 
       bool IsAcquire = IntNo == Intrinsic::arm_ldaexd;

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

def MCRR : MovRRCopro<"mcrr", 0 /* from ARM core register to coprocessor */,
                       (outs), (ins p_imm:$cop, imm0_15:$opc1, GPRnopc:$Rt,
                       GPRnopc:$Rt2, c_imm:$CRm),
                       [(int_arm_mcrr imm:$cop, imm:$opc1, GPRnopc:$Rt,
                                      GPRnopc:$Rt2, imm:$CRm)]>;
 def MRRC : MovRRCopro<"mrrc", 1 /* from coprocessor to ARM core register */,
                       (outs GPRnopc:$Rt, GPRnopc:$Rt2),
                       (ins p_imm:$cop, imm0_15:$opc1, c_imm:$CRm), []>;
 
-class MovRRCopro2<string opc, bit direction, list<dag> pattern = []>
-  : ABXI<0b1100, (outs), (ins p_imm:$cop, imm0_15:$opc1,
-         GPRnopc:$Rt, GPRnopc:$Rt2, c_imm:$CRm), NoItinerary,
+class MovRRCopro2<string opc, bit direction, dag oops, dag iops,
+                  list<dag> pattern = []>
+  : ABXI<0b1100, oops, iops, NoItinerary,
          !strconcat(opc, "\t$cop, $opc1, $Rt, $Rt2, $CRm"), pattern>,
     Requires<[PreV8]> {
   let Inst{31-28} = 0b1111;
   let Inst{23-21} = 0b010;
   let Inst{20} = direction;
 
   bits<4> Rt;
   bits<4> Rt2;
   bits<4> cop;
   bits<4> opc1;
   bits<4> CRm;
 
   let Inst{15-12} = Rt;
   let Inst{19-16} = Rt2;
   let Inst{11-8}  = cop;
   let Inst{7-4}   = opc1;
   let Inst{3-0}   = CRm;
 
-  let DecoderMethod = "DecodeMRRC2";
+  let DecoderMethod = "DecoderForMRRC2AndMCRR2";
 }
 
 def MCRR2 : MovRRCopro2<"mcrr2", 0 /* from ARM core register to coprocessor */,
+                        (outs), (ins p_imm:$cop, imm0_15:$opc1, GPRnopc:$Rt,
+                        GPRnopc:$Rt2, c_imm:$CRm),
                         [(int_arm_mcrr2 imm:$cop, imm:$opc1, GPRnopc:$Rt,
                                         GPRnopc:$Rt2, imm:$CRm)]>;
-def MRRC2 : MovRRCopro2<"mrrc2", 1 /* from coprocessor to ARM core register */>;
+
+def MRRC2 : MovRRCopro2<"mrrc2", 1 /* from coprocessor to ARM core register */,
+                       (outs GPRnopc:$Rt, GPRnopc:$Rt2),
+                       (ins p_imm:$cop, imm0_15:$opc1, c_imm:$CRm), []>;
 
 //===----------------------------------------------------------------------===//
 // Move between special register and ARM core register
 //
 
 // Move to ARM core register from Special Register
 def MRS : ABI<0b0001, (outs GPRnopc:$Rd), (ins), NoItinerary,
               "mrs", "\t$Rd, apsr", []> {

llvm/trunk/lib/Target/ARM/Disassembler/ARMDisassembler.cpp

static DecodeStatus DecodeT2Adr(MCInst &Inst, unsigned Val,
                                 uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeT2LdStPre(MCInst &Inst, unsigned Val,
                                 uint64_t Address, const void *Decoder);
 static DecodeStatus DecodeT2ShifterImmOperand(MCInst &Inst, unsigned Val,
                                 uint64_t Address, const void *Decoder);
 
 static DecodeStatus DecodeLDR(MCInst &Inst, unsigned Val,
                                 uint64_t Address, const void *Decoder);
-static DecodeStatus DecodeMRRC2(llvm::MCInst &Inst, unsigned Val,
+static DecodeStatus DecoderForMRRC2AndMCRR2(llvm::MCInst &Inst, unsigned Val,
                                             uint64_t Address, const void *Decoder);
 #include "ARMGenDisassemblerTables.inc"
 
 static MCDisassembler *createARMDisassembler(const Target &T,
                                              const MCSubtargetInfo &STI,
                                              MCContext &Ctx) {
   return new ARMDisassembler(STI, Ctx);
 }
 
static DecodeStatus DecodeLDR(MCInst &Inst, unsigned Val,
   if (!Check(S, DecodePostIdxReg(Inst, Rm, Address, Decoder)))
     return MCDisassembler::Fail;
   if (!Check(S, DecodePredicateOperand(Inst, Cond, Address, Decoder)))
     return MCDisassembler::Fail;
 
   return S;
 }
 
-static DecodeStatus DecodeMRRC2(llvm::MCInst &Inst, unsigned Val,
+static DecodeStatus DecoderForMRRC2AndMCRR2(llvm::MCInst &Inst, unsigned Val,
                                             uint64_t Address, const void *Decoder) {
 
   DecodeStatus S = MCDisassembler::Success;
 
   unsigned CRm = fieldFromInstruction(Val, 0, 4);
   unsigned opc1 = fieldFromInstruction(Val, 4, 4);
   unsigned cop = fieldFromInstruction(Val, 8, 4);
   unsigned Rt = fieldFromInstruction(Val, 12, 4);
   unsigned Rt2 = fieldFromInstruction(Val, 16, 4);
 
   if ((cop & ~0x1) == 0xa)
     return MCDisassembler::Fail;
 
   if (Rt == Rt2)
     S = MCDisassembler::SoftFail;
 
+  // We have to check if the instruction is MRRC2
+  // or MCRR2 when constructing the operands for
+  // Inst. Reason is because MRRC2 stores to two
+  // registers so it's tablegen desc has has two
+  // outputs whereas MCRR doesn't store to any
+  // registers so all of it's operands are listed
+  // as inputs, therefore the operand order for
+  // MRRC2 needs to be [Rt, Rt2, cop, opc1, CRm]
+  // and MCRR2 operand order is [cop, opc1, Rt, Rt2, CRm]
+
+  if (Inst.getOpcode() == ARM::MRRC2) {
+    if (!Check(S, DecodeGPRnopcRegisterClass(Inst, Rt, Address, Decoder)))
+      return MCDisassembler::Fail;
+    if (!Check(S, DecodeGPRnopcRegisterClass(Inst, Rt2, Address, Decoder)))
+      return MCDisassembler::Fail;
+  }
   Inst.addOperand(MCOperand::createImm(cop));
   Inst.addOperand(MCOperand::createImm(opc1));
+  if (Inst.getOpcode() == ARM::MCRR2) {
     if (!Check(S, DecodeGPRnopcRegisterClass(Inst, Rt, Address, Decoder)))
       return MCDisassembler::Fail;
     if (!Check(S, DecodeGPRnopcRegisterClass(Inst, Rt2, Address, Decoder)))
       return MCDisassembler::Fail;
+  }
   Inst.addOperand(MCOperand::createImm(CRm));
 
   return S;
 }
 

llvm/trunk/test/CodeGen/ARM/intrinsics-coprocessor.ll

entry:
   ; CHECK: stc p7, c3, [r{{[0-9]+}}]
   tail call void @llvm.arm.stc(i32 7, i32 3, i8* %i) nounwind
   ; CHECK: stcl p7, c3, [r{{[0-9]+}}]
   tail call void @llvm.arm.stcl(i32 7, i32 3, i8* %i) nounwind
   ; CHECK: stc2 p7, c3, [r{{[0-9]+}}]
   tail call void @llvm.arm.stc2(i32 7, i32 3, i8* %i) nounwind
   ; CHECK: stc2l p7, c3, [r{{[0-9]+}}]
   tail call void @llvm.arm.stc2l(i32 7, i32 3, i8* %i) nounwind
+  ; CHECK: mrrc p1, #2, r{{[0-9]+}}, r{{[0-9]+}}, c3
+  %2 = tail call { i32, i32 } @llvm.arm.mrrc(i32 1, i32 2, i32 3) nounwind
+  ; CHECK: mrrc2 p1, #2, r{{[0-9]+}}, r{{[0-9]+}}, c3
+  %3 = tail call { i32, i32 } @llvm.arm.mrrc2(i32 1, i32 2, i32 3) nounwind
   ret void
 }
 
 declare void @llvm.arm.ldc(i32, i32, i8*) nounwind
 
 declare void @llvm.arm.ldcl(i32, i32, i8*) nounwind
 
 declare void @llvm.arm.ldc2(i32, i32, i8*) nounwind

 
 declare void @llvm.arm.mcr2(i32, i32, i32, i32, i32, i32) nounwind
 
 declare i32 @llvm.arm.mrc2(i32, i32, i32, i32, i32) nounwind
 
 declare void @llvm.arm.mcr(i32, i32, i32, i32, i32, i32) nounwind
 
 declare i32 @llvm.arm.mrc(i32, i32, i32, i32, i32) nounwind
+
+declare { i32, i32 } @llvm.arm.mrrc(i32, i32, i32) nounwind
+
+declare { i32, i32 } @llvm.arm.mrrc2(i32, i32, i32) nounwind