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

Fri Oct 15 06:52:33 PDT 2021

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

================
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:
> frasercrmck wrote:
> > 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.
> Yeah if the alignment is >= than the size of the widened load it assumes it won't fault.
> 
> 
> An easy fix might be to squash the alignment to 0 for scalable vectors here. I assume that's why volatile crashes.
> 
> ```
>   // Allow wider loads if they are sufficiently aligned to avoid memory faults   
>   // and if the original load is simple.                                         
>   unsigned LdAlign = (!LD->isSimple()) ? 0 : LD->getAlignment();
> ```
I've (finally) created D111885 to fix the wrong code, and to serve as a basis for this work.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111248