Page MenuHomePhabricator

[ARM] Add MVE vector bit-operations (register inputs).
ClosedPublic

Authored by simon_tatham on May 30 2019, 8:21 AM.

Details

Summary

This includes all the obvious bitwise operations (AND, OR, BIC, ORN,
MVN); byte-order reverse instructions; and the VMOVs that access a
single lane of a vector.

Some of those VMOVs (specifically, the ones that access a 32-bit lane)
share an encoding with existing instructions that were disassembled as
accessing half of a d-register (e.g. vmov.32 r0, d1[0]), but in
8.1-M they're now written as accessing a quarter of a q-register (e.g.
vmov.32 r0, q0[2]). The older syntax is still accepted by the
assembler.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.May 30 2019, 8:21 AM

Remastered patch to apply cleanly against current trunk.

miyuki added a subscriber: miyuki.Jun 11 2019, 5:54 AM

Updated to current trunk, and renamed all the instruction classes in line with the new policy of having them all begin MVE_ for consistency.

ostannard added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
1306 ↗(On Diff #205520)

"size" is a 2-bit value, so "size{1-0}" can be simplified to just "size". (repeated in this file)

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
1947 ↗(On Diff #205520)

Do we need a new function for this, rather than re-using the NEON ones above?

6679 ↗(On Diff #205520)

Which operand require special parsing? This comment sounds like it's referring to operands with a custom ParserMethod listed in the tablegen description, but this patch doesn't add any of those.

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6053 ↗(On Diff #205520)

This could do with a comment explaining the two encodings we're converting between (especially the MCOperand one, as that's not documented elsewhere).

simon_tatham marked 2 inline comments as done.Wed, Jun 19, 7:52 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
1947 ↗(On Diff #205520)

I think the point of this one is that it takes the size as a template parameter, which in turn can be passed through from the Tablegen template parameter on the MVEVectorIndexOperand class.

I could rename it to isVectorIndex and replace existing uses of isVectorIndex32 with isVectorIndex<2>, and similarly with 64 and <1>?

6679 ↗(On Diff #205520)

That comment refers back ("as mentioned earlier") to one further up this function, which mentions that the potential problem is that custom parser methods might refer to fixed offsets in the vector of parsed operands.

An example is the NEON VMOV instruction that takes an FP immediate. The custom parser method for that operand is parseFPImm`, which checks Operands[2] to find out the width of the FP lane (expecting that operand to be a string such as .f16).

I think the point of this business is that we're trying not to perturb parsing of the existing instructions with the same mnemonic, by making sure the operand vector looks the same way it used to during parsing, and only inserting the new MVE predicate after all of that stuff has finished running.

Addressed minor review comments (isVectorIndex family, pointless
suffixes on size{1-0} and similar). Also, fixed the incomprehensible
mess of MVE vector indices by rewriting the MVE_VMOV_lane class
hierarchy completely: by splitting it up in the other order (first by
lane size, then by transfer direction) I managed to arrange that the
lane index operand becomes a perfectly normal integer which doesn't
even need custom encode/decode methods at all.

This revision is now accepted and ready to land.Wed, Jun 19, 9:20 AM
This revision was automatically updated to reflect the committed changes.