This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add support for MVE pre and post inc loads and stores.
ClosedPublic

Authored by dmgreen on Jun 26 2019, 1:35 PM.

Details

Summary

This adds pre- and post- increment and decrements for MVE loads and stores. It uses the builtin pre and post load/store detection, unlike Neon. Loads are selected with the code in tryT2IndexedLoad, stores are selected with tablegen patterns. The immediates have a +/-7bit range, multiplied by the size of the element.

Diff Detail

Event Timeline

dmgreen created this revision.Jun 26 2019, 1:35 PM
SjoerdMeijer added inline comments.Jun 27 2019, 4:00 AM
llvm/include/llvm/Target/TargetSelectionDAG.td
1122

nit: indent off by 1

1164

here

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
150

and here

152

and here

that's it, just nitpicking! :-)

llvm/lib/Target/ARM/ARMISelLowering.cpp
355

perhaps we could have a little bit of fun with c++ and do something like this:

for (unsigned I : ISD::pre_inc_ins())

where pre_inc_ins() creates an iterator range, similar to what happens in MachineValueType.h

dmgreen updated this revision to Diff 209840.Jul 15 2019, 6:49 AM

The original version wasn't getting alignment correct in some cases. For little endian which instruction we choose has more to do with alignment and offset than type being loaded. I've rewritten parts of this to, at least for LE, get this hopefully more correct. I've tried to add BE too, but not added any tests for that yet. We still have a task to sort out BE properly.

SjoerdMeijer added inline comments.Jul 26 2019, 1:26 AM
llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
1610

bail here for BE?

1612

Why the >= comparisons for the alignment? Is that right?

1631

I am wondering if this is logically correct, the (IsLE || ... in particular.
Perhaps we don't need the IsLE check here, if we bail for BE earlier.

dmgreen added inline comments.Jul 28 2019, 11:16 AM
llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
1610

We still need to select instructions for BE.

Granted I can't guarantee this will work correctly there yet, I have to go through and make sure all BE code works.

1612

The instruction (VLDRH) only supports alignments >= 2. Anything more than 2 is fine though (presuming it's a power of 2).

1631

The idea is that so long as we are LE, all the VLDRX instructions load data into the same lanes, so any can be used. The VLDRB.u8 will load the same data into the same place as a VLDRW.u32, just with a lower alignment constraint and a lower immediate range.

In BE though, they will reverse the values as they are loaded into the lanes, so the types do actually become important.

SjoerdMeijer accepted this revision.Aug 5 2019, 8:35 AM

Looks reasonable

llvm/include/llvm/Target/TargetSelectionDAG.td
1122

same nit if I'm not mistaken

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
1610

Okay, got it. Given you're still working on BE, this looks okay.

1612

I was actually thinking about that, can we assume it's a power of 2?

1631

okay, got it, cheers.

This revision is now accepted and ready to land.Aug 5 2019, 8:35 AM
dmgreen marked an inline comment as done.Aug 5 2019, 10:06 AM
dmgreen added inline comments.
llvm/include/llvm/Target/TargetSelectionDAG.td
1122

Sorry, I did mean to get to this! I need to rebase this over the bigendian code too. Let me do that now.

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp
1612

I believe alignments are always powers of 2. They can be 0 in IR, but then the abi alignment will be used by this point (which is 8 for these vectors, IIRC).

dmgreen updated this revision to Diff 213399.Aug 5 2019, 10:28 AM

Formatting and rebased over D65580 and D65583.

This revision was automatically updated to reflect the committed changes.