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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

Looks reasonable



================
Comment at: llvm/include/llvm/Target/TargetSelectionDAG.td:1114
+def pre_truncstvi16 : PatFrag<(ops node:$val, node:$base, node:$offset),
+                             (pre_truncst node:$val, node:$base, node:$offset)> {
+  let IsStore = 1;
----------------
SjoerdMeijer wrote:
> nit: indent off by 1
same nit if I'm not mistaken


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:1610
+  unsigned Align = LD->getAlignment();
+  bool IsLE = Subtarget->isLittle();
+
----------------
dmgreen wrote:
> 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.
Okay, got it. Given you're still working on BE, this looks okay.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:1612
+
+  if (Align >= 2 && LoadedVT == MVT::v4i16 &&
+      SelectT2AddrModeImm7Offset(N, LD->getOffset(), Offset, 1)) {
----------------
dmgreen wrote:
> 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).
I was actually thinking about that, can we assume 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))
----------------
dmgreen wrote:
> 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.
okay, got it, cheers.


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

https://reviews.llvm.org/D63840