This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Code-generation infrastructure for MVE.
ClosedPublic

Authored by simon_tatham on Apr 15 2019, 6:00 AM.

Details

Summary

This provides the low-level support to start using MVE vector types in
LLVM IR, loading and storing them, passing them to __asm__ statements
containing hand-written MVE vector instructions, and *if* you have the
hard-float ABI turned on, using them as function parameters.

(In the soft-float ABI, vector types are passed in integer registers,
and combining all those 32-bit integers into a q-reg requires support
for selection DAG nodes like insert_vector_elt and build_vector which
aren't implemented yet for MVE.)

Specifically, this commit adds support for:

  • spills, reloads and register moves for MVE vector registers
  • ditto for the VPT predication mask that lives in VPR.P0
  • make all the MVE vector types legal in ISel, and provide selection DAG patterns for BITCAST, LOAD and STORE
  • make loads and stores of scalar FP types conditional on hasFPRegs() rather than hasVFP2Base(). As a result a few existing tests needed their llc command lines updating to use -mattr=-fpregs as their method of turning off all hardware FP support.

Diff Detail

Repository
rL LLVM

Event Timeline

simon_tatham created this revision.Apr 15 2019, 6:00 AM

Can any of this be tested yet, or are we still missing some patches?

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
809 ↗(On Diff #195169)

Style nit: opening brace should be on the line above.

933 ↗(On Diff #195169)

Should we assert that the other register is a GPR here?

llvm/lib/Target/ARM/Thumb2ITBlockPass.cpp
206 ↗(On Diff #195169)

Does anything generate VPT-predicated instructions yet? If not, this should probably be split into a separate patch.

t.p.northover added inline comments.
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
808–809 ↗(On Diff #195169)

Braces go on the same line as the ) in LLVM style.

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
1284–1286 ↗(On Diff #195169)

I think this would be clearer if you used the isInt<7> functions from MathExtras.h.

llvm/lib/Target/ARM/Thumb2ITBlockPass.cpp
208–209 ↗(On Diff #195169)

I think this function would be a good place to assert that all instructions have at most one kind of predication.

300 ↗(On Diff #195169)

This continue isn't needed with the new control flow (and is inconsistent with the other branches).

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
694 ↗(On Diff #195169)

Possibly better as a static helper; then you don't have to explain it at all in the documentation comments in the header.

simon_tatham marked an inline comment as done.Apr 16 2019, 8:42 AM

This infrastructure should be enough to let you use the vector types in C as a means of getting them to and from __asm__ blocks that do the real work. I suppose that means I should include some tests that exercise exactly that, using asm operations in IR.

llvm/lib/Target/ARM/Thumb2ITBlockPass.cpp
206 ↗(On Diff #195169)

You have a point – predicated instructions won't be generated by anything I can think of in this patch series, and if you put one in an __asm__ block then it won't be represented in a way that this phase will notice.

So yes, perhaps I should back out this part of the patch until the ACLE intrinsics implementation is ready for review.

Updated this patch so that it doesn't depend on the intrusive calling convention changes in D60707 (which no longer looks necessary, so I'm probably going to abandon it).

simon_tatham edited the summary of this revision. (Show Details)Jun 3 2019, 8:35 AM
ostannard accepted this revision.Jun 4 2019, 2:22 AM

There is still a lot of code here not tested, but I think it will all be easier to test once we have some more actual instruction selection, so LGTM.

This revision is now accepted and ready to land.Jun 4 2019, 2:22 AM
simon_tatham edited the summary of this revision. (Show Details)

Remastered patch to apply cleanly against current trunk.

dmgreen added inline comments.Jun 24 2019, 4:33 AM
llvm/test/CodeGen/Thumb2/mve-basic.ll
2 ↗(On Diff #204038)

Can you also add a -mattr=+mve run line?

miyuki added a subscriber: miyuki.Jun 25 2019, 6:06 AM

Revised this patch to work properly with all the other MVE-related changes we've been making.

In particular, I've incorporated the whole of D63252 into this patch, because after I added a RUN line to the new mve-basic.ll test to try it in integer-only MVE, I found the bitconvert selection patterns from that patch were needed to get it to pass.

A few other changes: I made all the vector types legal even in integer-only MVE (in that you can load, store and bitconvert them, but nothing else), which is necessary so that the calling convention can work (since it converts everything to v2f64, so that has to be legal!). I added a missing case in copyPhysReg, which I had previously switched from the NEON VORRq instruction to MVE_VORR for the purposes of copying a single q-reg, but forgot to do the same for copying multiple q-regs (the QQPR or QQQQPR reg classes). Integer MVE support also needed me to legalise the scalar FP types, so I changed the existing condition for that to hasFPRegs in place of hasVFP2Base, which had knock-on effects on a couple of existing tests.

simon_tatham edited the summary of this revision. (Show Details)Jun 25 2019, 8:11 AM
dmgreen accepted this revision.Jun 25 2019, 9:15 AM

Nice one

llvm/test/CodeGen/Thumb2/mve-bitcasts.ll
2 ↗(On Diff #206454)

This test might as well get a -mattr=+mve line too, if we expect the output to be the same.

This revision was automatically updated to reflect the committed changes.