[PATCH] D21408: [ARM] MRRC2 shouldn't allow predicates

[Ranjeet Singh via llvm-commits]

rs updated this revision to Diff 60981.
rs added a comment.

> 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


http://reviews.llvm.org/D21408

Files:
  lib/Target/ARM/ARMISelDAGToDAG.cpp

Index: lib/Target/ARM/ARMISelDAGToDAG.cpp
===================================================================
--- lib/Target/ARM/ARMISelDAGToDAG.cpp
+++ lib/Target/ARM/ARMISelDAGToDAG.cpp
@@ -3350,8 +3350,15 @@
       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 */
-      Ops.push_back(getAL(CurDAG, dl));
-      Ops.push_back(CurDAG->getRegister(0, MVT::i32));
+
+      // 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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D21408.60981.patch
Type: text/x-patch
Size: 1122 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160616/7ff9a570/attachment.bin>