[PATCH] D148647: [RISCV] Fix interleave crash on unary interleaves

[Luke Lau via Phabricator via llvm-commits]

luke added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3297
+  int NumEltsPerOp = NumElts / 2;
+
+  int MaskRange;
----------------
craig.topper wrote:
> I think you maybe you want
> 
> I think maybe you can do something like
> 
> ```
> int HalfNumElts = NumElts / 2;
> return (((EvenSrc % NumElts) + HalfNumElts) <= NumElts) &&
>             (((OddSrc % NumElts) + HalfNumElts) <= NumElts);
> ```
Thanks, that's much better


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3294
 
+  // If both elements of the even and odd vector are smaller than the size of
+  // the VT, then it's an unary interleave like:  
----------------
craig.topper wrote:
> luke wrote:
> > craig.topper wrote:
> > > I wonder if the fix should be
> > > 
> > > if (EvenSrc % (NumElts / 2) != 0 || OddSrc % (NumElts /2) != 0)
> > That works for the first two cases mentioned but then it seems to miss the interleave in this case:
> > 
> > ```
> > ; This interleaves the first 2 elements of a vector in opposite order. With
> > ; undefs for the remaining elements. We use to miscompile this.
> > define <4 x i8> @unary_interleave_10uu_v4i8(<4 x i8> %x) {
> >   %a = shufflevector <4 x i8> %x, <4 x i8> poison, <4 x i32> <i32 1, i32 0, i32 undef, i32 undef>
> >   ret <4 x i8> %a
> > }
> > ```
> > 
> > I noticed we also probably want to support non-multiple-of-numelts offsets like this:
> > ```
> > shufflevector <4 x i32> %x, <4 x i32> %y, <4 x i32> <i32 0, i32 5, i32 1, i32 6>
> > ```
> > 
> > I've updated the patch to check the range given we know the start indices and size of each vector being interleaved, but there's probably a more elegant way of expressing this that I'm missing
> Does that case in `unary_interleave_10uu_v4i8` violate this rule for the extract_subvector?
> 
> ```
>   /// EXTRACT_SUBVECTOR(VECTOR, IDX) - Returns a subvector from VECTOR.          
>   /// Let the result type be T, then IDX represents the starting element number  
>   /// from which a subvector of type T is extracted. IDX must be a constant      
>   /// multiple of T's known minimum vector length.
> ```
It does, `EvenSrc` is 1 and size is 4 so it generates `t16: v2i8 = extract_subvector t4, Constant:i64<1>`. Should there not be an assertion triggering somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148647