[PATCH] D102493: [RISCV] Expand unaligned fixed-length vector memory accesses

Thu May 20 01:25:46 PDT 2021

frasercrmck added a comment.

In D102493#2769132 <https://reviews.llvm.org/D102493#2769132>, @craig.topper wrote:

> In D102493#2768973 <https://reviews.llvm.org/D102493#2768973>, @frasercrmck wrote:
>
>> I hadn't actually considered that. It should work in theory, but we do currently have a cap on the size of the legal vector types so this wouldn't currently work with something like `<128 x i32>`. We could maybe do it pre-legalization, but I don't know if it'd get legalized right back to the unaligned version. The good news it'd presumably work for scalable vectors too. Do you know of other targets doing this?
>
> Can we add the missing MVT types or cap the vXi32/i64 vectors we support to the same total with as the longest vXi8 type?

I have a task to look into this which I may bring forward. I think this is what we want to see, I just hope we don't have too many legalization issues at the type "limits" like that shuffle I initially tried to fix by legalizing `any_extend_vector_inreg`.

>> Getting masked intrinsics to work via bitcasts may take a bit of doing though. I suppose you'd need to shuffle the indices and add on extra byte offsets like `<0,1,2,3,0,1,...>`. I don't know if that's safe to do since you may risk overflow.
>
> You would also need to duplicate bits in the mask which isn't straight forward.

Indeed.

>> @craig.topper can you see anything I've missed here?
>
> The regular load/store case is the most interesting because InstCombine merges bitcasts into loads/stores if I recall correctly. So it might create a bad case without realizing it.

Yeah. Though the test case that uncovered this bug for us is a masked scatter, but that was the vectorizer's choice which is slightly different.

In D102493#2769239 <https://reviews.llvm.org/D102493#2769239>, @efriedma wrote:

> We end up doing this sort of thing on ARM in some cases.  For example https://reviews.llvm.org/D100527 , https://reviews.llvm.org/D70176 .

Thanks for the pointers! Pun -- sadly -- intended.

I think if it's okay by you, @craig.topper, I'll merge this as-is (since we need the bug fix) and start working on the improvement patch. It seems like we may need to get some other changes in before the bitcasting will work across the board.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102493