This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add MVE vector load/store instructions.
ClosedPublic

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

Details

Summary

This adds the full set of vector memory access instructions. It
includes contiguous loads/stores, with an ordinary addressing mode
such as [r0,#offset] (plus writeback variants); gather loads and
scatter stores with a scalar base address register and a vector of
offsets from it (written [r0,q1] or similar); and gather/scatters with
a vector of base addresses (written [q0,#offset], again with
writeback). Additionally, some of the loads can widen each loaded
value into a larger vector lane, and the corresponding stores narrow
them again. Finally, there's the VLD2 / VLD4 family, which distributes
2 or 4 vectors' worth of memory data across the lanes of the same
number of registers but in a transposed order.

To implement these, we also have to add the addressing modes they
need, and the register list operands used by VLD2/VST4. Also, in
AsmParser, the isMem query function now has subqueries isGPRMem
and isMVEMem, according to which kind of base register is used by a
given memory access operand.

Diff Detail

Repository
rL LLVM

Event Timeline

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

Removed some code duplication in ARMMCCodeEmitter.cpp, arising from review of D62667.

samparker added inline comments.Jun 4 2019, 2:25 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
3483 ↗(On Diff #202446)

These descriptions would be far easier to read if the load and store were separated. That would also make them easier to use when we want to apply isel patterns. I can't say that I've seen this syntax used in this way before... probably for good reason.

3558 ↗(On Diff #202446)

Looks like these mayLoad/Store flags have been mixed up.

3609 ↗(On Diff #202446)

Again, I'm assuming that this incorrect, IndexModeNone instead?

3694 ↗(On Diff #202446)

and here.

3731 ↗(On Diff #202446)

What is f8?

3845 ↗(On Diff #202446)

Should this 'ins' be passed as an argument? Looks like all the instructions pass it.

3955 ↗(On Diff #202446)

Since we're already passing 'shift', it would be cleaner to construct the addrOps within the multiclass definitions.

llvm/lib/Target/ARM/ARMInstrThumb2.td
313 ↗(On Diff #202446)

Could this stuff be moved into the MVE specific td file?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
1776 ↗(On Diff #202446)

Please extract out the shared logic into a helper.

2636 ↗(On Diff #202446)

either use cast if you're sure, otherwise we need to ensure CE isn't nullptr.

3063 ↗(On Diff #202446)

Do we really need all these functions that do exactly the same thing?!

4846 ↗(On Diff #202446)

NoLanes and AllLanes can be folded.

7628 ↗(On Diff #202446)

Please use a helper to avoid the copy-paste.

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
6357 ↗(On Diff #202446)

This functions share a lot of common functionality, please refactor.

Remastered patch to apply cleanly against current trunk.

miyuki added a subscriber: miyuki.Jun 11 2019, 5:54 AM
simon_tatham marked 2 inline comments as done.Jun 20 2019, 6:35 AM

(I'm still working on the rest of your review comments)

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
2636 ↗(On Diff #202446)

That hardly seems fair – all the other functions on both sides of this hunk are written in the same style! But I'll add an assertion to just these ones if you like :-)

3063 ↗(On Diff #202446)

I think all the names have to exist, because the Tablegen-generated code will want to call them all by concatenating the innermost operand name (Imm7Shift1 or similar) into the middle of a standard function-name template. But I can turn them all into trivial wrappers on a single function body.

ostannard added inline comments.
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
3063 ↗(On Diff #202446)

You can override the function name used by setting the RenderMethod field of the AsmOperandClass, so these could all use the same function.

I've decided this patch is too large to manage all in one go. Also, the interleaving family of loads (VLD20 and friends) share essentially no infrastructure with the VLDR family, so that seems like a natural place to split the patch in two.

Hence, I've just opened D63650, containing just the VLD20 family, hopefully with all comments on this patch taken into account. I'll keep this review open and use it for the remainder of the loads and stores, but I haven't finished reworking it yet.

Reworked the remaining loads and stores to address review comments, implement consistent naming of instruction ids, and tidy up the Tablegen so that it's hopefully halfway readable.

I think this is still the most complicated subset of instructions in the whole of MVE, but I'm running out of ways I can make it simpler.

Minor revisions to this patch: changed a couple of legacy t2rGPR into rGPR (after the former was withdrawn during code review of D63650). Also added a small knock-on fix in checkTargetMatchPredicate, preventing an assertion failure in a check that was specific to rGPR.

ostannard added inline comments.Jun 24 2019, 9:25 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
161 ↗(On Diff #206203)

I think this can be written like this, without the defs above:

let ParserMatchClass = TMemImm7ShiftOffsetAsmOperand<shift>;
3611 ↗(On Diff #206203)

Why is this needed, could we just define instances of MVE_VLDRSTR_rq directly below?

llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
5812 ↗(On Diff #206203)

Missing space before equals

7487 ↗(On Diff #206235)

I don't see any tests for the QmIsPointer version of this error.

llvm/lib/Target/ARM/Disassembler/ARMDisassembler.cpp
4246 ↗(On Diff #206203)

This doesn't need to be on it's own line.

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
617 ↗(On Diff #206235)

I think it would be clearer to define the values for all three AddrModes in the switch, rather than using these defaults for one of them.

llvm/test/MC/ARM/mve-load-store.s
2 ↗(On Diff #206235)

None of these instructions differ with mve.fp (unless I missed some), so there's no need to duplicate the check lines. We should also be checking the error messages in the no-fp case.

simon_tatham marked 5 inline comments as done.Jun 25 2019, 1:45 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
3611 ↗(On Diff #206203)

The idea was partly to abstract away all the parameters to MVE_VLDRSTR_rq that are common between the byte variants, and mostly to make all the defms look similar enough that the reader can focus on only the necessary differences.

Perhaps a one-element defm was silly and I should turn it into an ordinary subclass, though.

Addressed all review comments, I think.

This revision is now accepted and ready to land.Jun 25 2019, 2:50 AM
This revision was automatically updated to reflect the committed changes.
kcc added a subscriber: kcc.Jun 27 2019, 1:41 PM

This new test is currently failing on the ubsan build.
Could you please take a look?
I don't yet know is the breaking change.

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/33133/steps/check-llvm%20ubsan/logs/stdio

FAIL: LLVM :: MC/Disassembler/ARM/mve-load-store.txt (22713 of 32124)

  • TEST 'LLVM :: MC/Disassembler/ARM/mve-load-store.txt' FAILED ****

Script:

: 'RUN: at line 1'; /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -mattr=+mve.fp,+fp64 -show-encoding /b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt 2> /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/test/MC/Disassembler/ARM/Output/mve-load-store.txt.tmp | /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/FileCheck /b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt
: 'RUN: at line 2'; /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/FileCheck --check-prefix=ERROR < /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/test/MC/Disassembler/ARM/Output/mve-load-store.txt.tmp /b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt
: 'RUN: at line 3'; not /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/llvm-mc -disassemble -triple=thumbv8.1m.main-none-eabi -show-encoding /b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt &> /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/test/MC/Disassembler/ARM/Output/mve-load-store.txt.tmp

: 'RUN: at line 4'; /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/bin/FileCheck --check-prefix=CHECK-NOMVE < /b/sanitizer-x86_64-linux-fast/build/llvm_build_ubsan/test/MC/Disassembler/ARM/Output/mve-load-store.txt.tmp /b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt

Exit Code: 1

Command Output (stderr):

/b/sanitizer-x86_64-linux-fast/build/llvm/test/MC/Disassembler/ARM/mve-load-store.txt:269:10: error: CHECK: expected string not found in input

CHECK: vldrb.u32 q0, [r0] @ encoding: [0x90,0xfd,0x00,0x0f]

^

<stdin>:68:2: note: scanning from here
vldr
^

kcc added a comment.Jun 27 2019, 2:01 PM

Actually, the test started failing in ubsan mode right after this commit. Please fix ASAP. Reproduce: build with -DLLVM_USE_SANITIZER=Undefined, then "ninja check-llvm"

Sorry about that. rL364635 should fix it.