[PATCH] D153864: [RISCV] Lower interleave2 intrinsics to vsseg2

[Philip Reames via Phabricator via llvm-commits]

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

LGTM

I'm fine with the alignment piece being done after this lands.  It is a bug, but it's a bug we already have and should be very narrow.  Just make sure you tackle that in the next few days.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:16881
+                                                          StoreInst *SI) const {
+  IRBuilder<> Builder(SI);
+
----------------
Please add an assert that SI isSimple.  This is checked by the caller, but this could relies on it for correctness, so we should guard against future changes. 


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:16892
+
+  if (!isLegalInterleavedAccessType(InVTy, Factor,
+                                    SI->getModule()->getDataLayout()))
----------------
There's a missing legality check here.  It's actually missing in the lowerInterleavedStore as well.

If the store is misaligned (i.e. has an explicit align 1), then the resulting segmented store may not be legal.  For a processor which supports misaligned scalars, but not misaligned vectors (or merely not misaligned segment stores), this could be a problem.

Probably the best fix is to add an alignment parameter to isLegalInterleaveAccessType. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153864