[PATCH] D111248: [SelectionDAG] Widen scalable-vector stores via VP_STORE

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 2 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:5022
+  // Generate a vector-predicated store if it is custom/legal on the target.
+  // To avoid possible recusion, only do this if the widened mask type is
+  // legal.
----------------
craig.topper wrote:
> recusion -> recursion?
Good spot, thanks.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:5037
+  EVT WideVT = TLI.getTypeToTransformTo(*DAG.getContext(), StVT);
+  EVT WideMaskVT = WideVT.changeVectorElementType(MVT::i1);
+  if (WideVT.isScalableVector() &&
----------------
craig.topper wrote:
> I'm distrusting of changeVectorElementType. If WideVT happens to be an MVT, the changed type must also be an MVT since there is no way to get the LLVMContext from the MVT to make an EVT for the i1 vector.
> 
> Can we use EVT::getVectorVT here instead?
Quite right. It was my intention to fix that up before posting the patch, but I forgot


================
Comment at: llvm/test/CodeGen/RISCV/rvv/load-store-sdnode.ll:40
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    vsetvli a1, zero, e8, mf2, ta, mu
+; CHECK-NEXT:    vle8.v v8, (a0)
----------------
craig.topper wrote:
> How are we able to widen this load? Looks like we inferred an align of 4. I think we used that alignment to infer that it was ok to widen using the logic we have for fixed vectors. But I don't think that can work with vscale.
I'm glad you pointed this out. I was pretty surprised by this (I was expecting it to crash like `store`s do). I then somehow convinced myself that it was okay but that was probably wishful thinking. Is the assumption that the hardware's addressable memory is always a multiple of the alignment and so if `<3 x i8>` is aligned to 4 there must be an extra byte that it can load without faulting?

If so I agree that vscale doesn't work like this. Do you think this should be patched quickly to crash like stores currently do? Feels pretty blunt but it's better than silently generating wrong code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111248