Page MenuHomePhabricator

Add mrrc/mrrc2 co-processor intrinsics
ClosedPublic

Authored by rs on Jun 9 2016, 5:00 AM.

Details

Summary

This patch adds codegen support to generate the mrrc/mrrc2 instruction from the intrinsic.

Diff Detail

Repository
rL LLVM

Event Timeline

rs updated this revision to Diff 60160.Jun 9 2016, 5:00 AM
rs retitled this revision from to Add mrrc/mrrc2 co-processor intrinsics.
rs updated this object.
rs set the repository for this revision to rL LLVM.
rs added subscribers: t.p.northover, rengolin.
rs removed rL LLVM as the repository for this revision.
rs edited subscribers, added: llvm-commits; removed: rengolin, t.p.northover.
rengolin added inline comments.Jun 9 2016, 5:15 AM
lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5285 ↗(On Diff #60160)

This looks like a different change, for another patch.

Also, why only validate for MRRC and MCRR? Were the checks wrong for the rest?

If so, why not just have one check if MRRC || MCRR?

rengolin added inline comments.Jun 9 2016, 5:20 AM
lib/Target/ARM/ARMISelDAGToDAG.cpp
3303 ↗(On Diff #60160)

Bad name, use "isThumbV8M".

rs added a comment.Jun 9 2016, 6:30 AM

Thanks for the review Renato. I replied back to your comments below.

lib/Target/ARM/ARMISelDAGToDAG.cpp
3303 ↗(On Diff #60160)

Actually I think I only need to do Subtarget->isThumb() not the rest.

lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5285 ↗(On Diff #60160)

This looks like a different change, for another patch.

Decoder tests were failing because I changed the tablegen definitions for 'MCRR2' and 'MRRC2' to define the inputs and ouputs in 'lib/Target/ARM/ARMInstrInfo.td'.

Also, why only validate for MRRC and MCRR? Were the checks wrong for the rest?

This decoder method is only used by definitions instantiated from the tablegen class MovRRCopro2, there are only two definitions that use this class, they are MCRR2 and MRRC2..

rengolin added inline comments.Jun 9 2016, 9:10 AM
lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5285 ↗(On Diff #60160)

If only MCRR2 and MRRC2 use this method, there's no point for the condition, right?

rs added inline comments.Jun 9 2016, 9:22 AM
lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5285 ↗(On Diff #60160)

The conditional checks are for controlling the order in which the operands are added to Inst. For MRRC2, the order should be [Rt, Rt2, coproc, opc1, CRm] for MCRR2 it should be [coproc, opc1, Rt, Rt2, CRm]

rs added inline comments.Jun 10 2016, 3:36 AM
lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5285 ↗(On Diff #60160)

Just expanding on my previous answer. Previously the order in which variable Inst was constructed would be the same for both MRRC2 and MCRR2 because the tablgen description of the outputs and inputs was "...(outs), (ins p_imm:$cop, imm0_15:$opc1, GPRnopc:$Rt, GPRnopc:$Rt2, c_imm:$CRm...>". This tablegen is correct for MCRR2 but not MRRC2, MRRC2 writes to 2 registers so the tablegen description should be "...(outs GPRnopc:$Rt, GPRnopc:$Rt2), (ins p_imm:$cop, imm0_15:$opc1, c_imm:$CRm)...". This is why the operands in Inst for MRRC2 need to be [Rt, Rt2, coproc, opc1, CRm]

rengolin accepted this revision.Jun 10 2016, 8:24 AM
rengolin edited edge metadata.

Ok, LGTM with the { isThumb(), comment, function name } changes. Thanks!

lib/Target/ARM/Disassembler/ARMDisassembler.cpp
5268 ↗(On Diff #60160)

Can you give this a more generic name? Now it decodes both mrrc2 and mcrr2...

5285 ↗(On Diff #60160)

HA! Of course! Can you add a comment to that effect?

This revision is now accepted and ready to land.Jun 10 2016, 8:24 AM
rs added a comment.Jun 10 2016, 8:50 AM

Thanks for reviewing Renato, will make your suggested changes before committing. Can you also review the clang part of the patch at some point http://reviews.llvm.org/D21179 ?

Thanks,
Ranjeet

This revision was automatically updated to reflect the committed changes.