[PATCH] D63840: [ARM] Add support for MVE pre and post inc loads and stores.

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:1610
+  unsigned Align = LD->getAlignment();
+  bool IsLE = Subtarget->isLittle();
+
----------------
SjoerdMeijer wrote:
> bail here for BE?
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.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:1612
+
+  if (Align >= 2 && LoadedVT == MVT::v4i16 &&
+      SelectT2AddrModeImm7Offset(N, LD->getOffset(), Offset, 1)) {
----------------
SjoerdMeijer wrote:
> Why the >= comparisons for the alignment? Is that right?
The instruction (VLDRH) only supports alignments >= 2. Anything more than 2 is fine though (presuming it's a power of 2).


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:1631
+  } else if (Align >= 4 &&
+             (IsLE || LoadedVT == MVT::v4i32 || LoadedVT == MVT::v4f32) &&
+             SelectT2AddrModeImm7Offset(N, LD->getOffset(), Offset, 2))
----------------
SjoerdMeijer wrote:
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63840/new/

https://reviews.llvm.org/D63840