Page MenuHomePhabricator

[ARM] MRRC2 shouldn't allow predicates
ClosedPublic

Authored by rs on Jun 15 2016, 2:09 PM.

Details

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

Diff Detail

Event Timeline

rs updated this revision to Diff 60902.Jun 15 2016, 2:09 PM
rs retitled this revision from to [ARM] MRRC2 shouldn't allow predicates.
rs updated this object.
rs added a reviewer: rengolin.
rs added a subscriber: llvm-commits.
rengolin edited edge metadata.Jun 16 2016, 1:36 AM

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 updated this revision to Diff 60981.Jun 16 2016, 8:26 AM
rs edited edge metadata.

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 accepted this revision.Jun 16 2016, 10:14 AM
rengolin edited edge metadata.
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!

This revision is now accepted and ready to land.Jun 16 2016, 10:14 AM
This revision was automatically updated to reflect the committed changes.