Page MenuHomePhabricator

[ARM] Add MVE vector shift instructions.
ClosedPublic

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

Details

Summary

This includes saturating and non-saturating shifts, both with
immediate shift count and with the shift counts given by another
vector register; VSHLC (in which the bits shifted out of each active
vector lane are shifted in to the next active lane); and also VMOVL,
which is enough like an immediate shift that it didn't fit too badly
in this category.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.May 30 2019, 8:20 AM
ostannard added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
785 ↗(On Diff #202207)

Why are the R and Q registers in the opposite order in the input and output operands?

815 ↗(On Diff #202207)

Some of these names begin with "t2" (matching the other Thumb2 instructions), and some don't. Unless there's a pattern which I've missed, could pick one naming convention and stick to it?

909 ↗(On Diff #202207)

These ones seem to be be using the t1/t2 prefix to distinguish between different 32-bit encodings, rather than between the 16-bit and 32-bit encodings from the reference manual. I'd prefer if we avoided using the encoding numbers at all, because these don't tend to be stable between versions of the architecture.

1044 ↗(On Diff #202207)

This class is only used once, can it be merged into MVE_shift_helper?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5984 ↗(On Diff #202207)

What are these special cases for? Could this be moved into isMnemonicVPTPredicable?

6610 ↗(On Diff #202207)

Does this need to be an exact equality check, not just startswith? Otherwise, I think this will transform "vmov<whatever>lt" into just "vmovlt".

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6089 ↗(On Diff #202207)

Is this not already handled by the table-generated disassembler?

Remastered patch to apply cleanly against current trunk.

miyuki added a subscriber: miyuki.Jun 11 2019, 5:54 AM
simon_tatham marked 7 inline comments as done.Jun 18 2019, 7:11 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
785 ↗(On Diff #202207)

I think this one was intentional, because it reflects the logical order of the operand pairs. VSHLC shifts a Q-register left, filling in the rightmost bits from a GPR, and as output it produces a GPR full of the bits shifted off from the left. So you start with Qx,Ry which logically make up a single bit sequence if arranged in that order, and you end up with Ry,Qx which now make up a shifted version of the same bit sequence if arranged in that order.

815 ↗(On Diff #202207)

I suspect this was just a case of 'add a prefix wherever it's necessary to prevent a clash with an existing instruction'.

Some MVE instructions have already been committed, so I've opened D63492 to make those more consistent. Also revised D62671 in line with this comment. I'll try to arrange that all the MVE instructions have names beginning MVE_.

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5984 ↗(On Diff #202207)

I'm a bit unclear on this, but I think the problem here is that isMnemonicVPTPredicable has to be able to answer questions about the version of the mnemonic with a T or E suffix (because it's called by SplitMnemonic) and also about the version without one (because it's called by getMnemonicAcceptInfo after the split).

On that basis, these special cases are all things that isMnemonicVPTPredicable properly ought to return true for, either because they are things on which a T/E suffix would be legal (e.g. vqshrnt, in which the existing t means 'top half' for a narrowing instruction), or because they actually have one (vmovlt, unless that's a scalar vmov with an ordinary lt conditional suffix).

If we broke up isMnemonicVPTPredicable into two separate functions, with semantics "is this a mnemonic that might already include a T/E suffix?" and "is this a mnemonic that it would be legal to put a T/E suffix on the end of?", then we could probably get rid of most of these special cases ... (a) provided I haven't completely misunderstood all of this; (b) at the cost of making isMnemonicVPTPredicable itself hairier by more than we save here; (c) and it wouldn't even get rid of all of them, because I think the reason vmovlt is treated unusually here is to handle the lexing ambiguity between vmovl t and vmov lt.

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6089 ↗(On Diff #202207)

I think this custom decoder was written because of the combined size + shift count field in the encoding for these instructions. But I've found an alternative way to express it in pure Tablegen, so yes, it looks as if we can get rid of this.

Renamed all instructions for consistency; removed the unnecessary intermediate class; reworked VSHLL to manage without a custom C++ decoder function.

This revision is now accepted and ready to land.Jun 18 2019, 8:03 AM
Closed by commit rL363696: [ARM] Add MVE vector shift instructions. (authored by statham, committed by ). · Explain WhyJun 18 2019, 9:17 AM
This revision was automatically updated to reflect the committed changes.