[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!

lib/Target/ARM/ARMISelDAGToDAG.cpp

case Intrinsic::arm_mrrc2: {
         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);