[RISCV] Fix interleave crash on unary interleaves

[luke]

Summary

We were crashing when lowering interleave shuffles like
(shuffle <0,3,1,4>, x:v4i8, y:v4i8)
Where it was technically an unary shuffle (both EvenSrc and OddSrc point
to the first operand), but the resulting extract_subvectors were out of
bounds.
This checks to make sure that the vectors being extracted are within
range.

[craig.topper]

LGTM

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3283–3284

I think this needs to be int to avoid signed/unsigned comparison warnings against EvenSrc/OddSrc.

[craig.topper]

I think this made us no longer support

define <8 x i8> @foo(<8 x i8> %x, <8 x i8> %y) {                                 
  ; V128-LABEL: unary_interleave_v4i8_invalid:                                   
  ; V128:       # %bb.0:                                                         
  ; V128-NEXT:    lui a0, %hi(.LCPI17_0)                                         
  ; V128-NEXT:    addi a0, a0, %lo(.LCPI17_0)                                    
  ; V128-NEXT:    vsetivli zero, 4, e8, mf4, ta, ma                              
  ; V128-NEXT:    vle8.v v10, (a0)                                               
  ; V128-NEXT:    vrgather.vv v9, v8, v10                                        
  ; V128-NEXT:    vmv1r.v v8, v9                                                 
  ; V128-NEXT:    ret                                                            
  ;                                                                              
  ; V512-LABEL: unary_interleave_v4i8_invalid:                                   
  ; V512:       # %bb.0:                                                         
  ; V512-NEXT:    lui a0, %hi(.LCPI17_0)                                         
  ; V512-NEXT:    addi a0, a0, %lo(.LCPI17_0)                                    
  ; V512-NEXT:    vsetivli zero, 4, e8, mf8, ta, ma                              
  ; V512-NEXT:    vle8.v v10, (a0)                                               
  ; V512-NEXT:    vrgather.vv v9, v8, v10                                        
  ; V512-NEXT:    vmv1r.v v8, v9                                                 
  ; V512-NEXT:    ret                                                            
    %a = shufflevector <8 x i8> %x, <8 x i8> %y, <8 x i32> <i32 0, i32 12, i32 1, i32 13, i32 2, i32 14, i32 3, i32 15>
      ret <8 x i8> %a                                                            
}

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3297

I wonder if the fix should be

if (EvenSrc % (NumElts / 2) != 0 || OddSrc % (NumElts /2) != 0)

[luke]

Handle more cases where there may be an offset

[luke]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3297

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

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3297

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3299

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);

[luke]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

3297

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?

3299

Thanks, that's much better

[luke]

Address comments

[craig.topper]

LGTM

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

for (unsigned i = 1; i != Mask.size(); ++i)
     if (Mask[i] != Mask[i - 1] + 2)
       return false;
 
   return true;
 }
 
 /// Is this shuffle interleaving contiguous elements from one vector into the
 /// even elements and contiguous elements from another vector into the odd
-/// elements. \p Src1 will contain the element that should be in the first even
-/// element. \p Src2 will contain the element that should be in the first odd
-/// element. These can be the first element in a source or the element half
+/// elements. \p EvenSrc will contain the element that should be in the first
+/// even element. \p OddSrc will contain the element that should be in the first
+/// odd element. These can be the first element in a source or the element half
 /// way through the source.
 static bool isInterleaveShuffle(ArrayRef<int> Mask, MVT VT, int &EvenSrc,
                                 int &OddSrc, const RISCVSubtarget &Subtarget) {
   // We need to be able to widen elements to the next larger integer type.
   if (VT.getScalarSizeInBits() >= Subtarget.getELEN())
     return false;
 
   int Size = Mask.size();
-  assert(Size == (int)VT.getVectorNumElements() && "Unexpected mask size");
+  int NumElts = VT.getVectorNumElements();
+  assert(Size == (int)NumElts && "Unexpected mask size");
 
   SmallVector<unsigned, 2> StartIndexes;
   if (!ShuffleVectorInst::isInterleaveMask(Mask, 2, Size * 2, StartIndexes))
     return false;
 
   EvenSrc = StartIndexes[0];
   OddSrc = StartIndexes[1];
 
   // One source should be low half of first vector.
   if (EvenSrc != 0 && OddSrc != 0)
     return false;
 
-  return true;
+  // Subvectors will be subtracted from either at the start of the two input
+  // vectors, or at the start and middle of the first vector if it's an unary
+  // interleave.
+  // In both cases, HalfNumElts will be extracted.
+  // So make sure that EvenSrc/OddSrc are within range.
+  int HalfNumElts = NumElts / 2;
+  return (((EvenSrc % NumElts) + HalfNumElts) <= NumElts) &&
+         (((OddSrc % NumElts) + HalfNumElts) <= NumElts);
 }
 
 /// Match shuffles that concatenate two vectors, rotate the concatenation,
 /// and then extract the original number of elements from the rotated result.
 /// This is equivalent to vector.splice or X86's PALIGNR instruction. The
 /// returned rotation amount is for a rotate right, where elements move from
 /// higher elements to lower elements. \p LoSrc indicates the first source
 /// vector of the rotate or -1 for undef. \p HiSrc indicates the second vector

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-int-interleave.ll

 ; V512-NEXT:    li a0, -1
 ; V512-NEXT:    vwmaccu.vx v10, a0, v9
 ; V512-NEXT:    vmv1r.v v8, v10
 ; V512-NEXT:    ret
   %a = shufflevector <4 x i32> %x, <4 x i32> %y, <8 x i32> <i32 0, i32 4, i32 1, i32 5, i32 2, i32 6, i32 3, i32 7>
   ret <8 x i32> %a
 }
 
+; %y should be slid down by 2
+define <4 x i32> @interleave_v4i32_offset_2(<4 x i32> %x, <4 x i32> %y) {
+; V128-LABEL: interleave_v4i32_offset_2:
+; V128:       # %bb.0:
+; V128-NEXT:    vsetivli zero, 2, e32, m1, ta, ma
+; V128-NEXT:    vslidedown.vi v10, v9, 2
+; V128-NEXT:    vsetivli zero, 2, e32, mf2, ta, ma
+; V128-NEXT:    vwaddu.vv v9, v8, v10
+; V128-NEXT:    li a0, -1
+; V128-NEXT:    vwmaccu.vx v9, a0, v10
+; V128-NEXT:    vmv1r.v v8, v9
+; V128-NEXT:    ret
+;
+; V512-LABEL: interleave_v4i32_offset_2:
+; V512:       # %bb.0:
+; V512-NEXT:    vsetivli zero, 2, e32, mf2, ta, ma
+; V512-NEXT:    vslidedown.vi v10, v9, 2
+; V512-NEXT:    vwaddu.vv v9, v8, v10
+; V512-NEXT:    li a0, -1
+; V512-NEXT:    vwmaccu.vx v9, a0, v10
+; V512-NEXT:    vmv1r.v v8, v9
+; V512-NEXT:    ret
+  %a = shufflevector <4 x i32> %x, <4 x i32> %y, <4 x i32> <i32 0, i32 6, i32 1, i32 7>
+  ret <4 x i32> %a
+}
+
+; %y should be slid down by 1
+define <4 x i32> @interleave_v4i32_offset_1(<4 x i32> %x, <4 x i32> %y) {
+; V128-LABEL: interleave_v4i32_offset_1:
+; V128:       # %bb.0:
+; V128-NEXT:    vsetivli zero, 2, e32, m1, ta, ma
+; V128-NEXT:    vslidedown.vi v10, v9, 1
+; V128-NEXT:    vsetivli zero, 2, e32, mf2, ta, ma
+; V128-NEXT:    vwaddu.vv v9, v8, v10
+; V128-NEXT:    li a0, -1
+; V128-NEXT:    vwmaccu.vx v9, a0, v10
+; V128-NEXT:    vmv1r.v v8, v9
+; V128-NEXT:    ret
+;
+; V512-LABEL: interleave_v4i32_offset_1:
+; V512:       # %bb.0:
+; V512-NEXT:    vsetivli zero, 2, e32, mf2, ta, ma
+; V512-NEXT:    vslidedown.vi v10, v9, 1
+; V512-NEXT:    vwaddu.vv v9, v8, v10
+; V512-NEXT:    li a0, -1
+; V512-NEXT:    vwmaccu.vx v9, a0, v10
+; V512-NEXT:    vmv1r.v v8, v9
+; V512-NEXT:    ret
+  %a = shufflevector <4 x i32> %x, <4 x i32> %y, <4 x i32> <i32 0, i32 5, i32 1, i32 6>
+  ret <4 x i32> %a
+}
+
 define <16 x i8> @interleave_v8i8(<8 x i8> %x, <8 x i8> %y) {
 ; V128-LABEL: interleave_v8i8:
 ; V128:       # %bb.0:
 ; V128-NEXT:    vsetivli zero, 8, e8, mf2, ta, ma
 ; V128-NEXT:    vwaddu.vv v10, v8, v9
 ; V128-NEXT:    li a0, -1
 ; V128-NEXT:    vwmaccu.vx v10, a0, v9
 ; V128-NEXT:    vmv1r.v v8, v10

 ; RV32-V128-LABEL: interleave_v32i32:
 ; RV32-V128:       # %bb.0:
 ; RV32-V128-NEXT:    addi sp, sp, -16
 ; RV32-V128-NEXT:    .cfi_def_cfa_offset 16
 ; RV32-V128-NEXT:    csrr a0, vlenb
 ; RV32-V128-NEXT:    slli a0, a0, 4
 ; RV32-V128-NEXT:    sub sp, sp, a0
 ; RV32-V128-NEXT:    .cfi_escape 0x0f, 0x0d, 0x72, 0x00, 0x11, 0x10, 0x22, 0x11, 0x10, 0x92, 0xa2, 0x38, 0x00, 0x1e, 0x22 # sp + 16 + 16 * vlenb
-; RV32-V128-NEXT:    lui a0, %hi(.LCPI15_0)
-; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI15_0)
+; RV32-V128-NEXT:    lui a0, %hi(.LCPI17_0)
+; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI17_0)
 ; RV32-V128-NEXT:    li a1, 32
 ; RV32-V128-NEXT:    vsetvli zero, a1, e32, m8, ta, ma
 ; RV32-V128-NEXT:    vle32.v v0, (a0)
 ; RV32-V128-NEXT:    vmv8r.v v24, v8
 ; RV32-V128-NEXT:    vrgather.vv v8, v24, v0
 ; RV32-V128-NEXT:    addi a0, sp, 16
 ; RV32-V128-NEXT:    vs8r.v v24, (a0) # Unknown-size Folded Spill
-; RV32-V128-NEXT:    lui a0, %hi(.LCPI15_1)
-; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI15_1)
+; RV32-V128-NEXT:    lui a0, %hi(.LCPI17_1)
+; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI17_1)
 ; RV32-V128-NEXT:    vle32.v v24, (a0)
 ; RV32-V128-NEXT:    csrr a0, vlenb
 ; RV32-V128-NEXT:    slli a0, a0, 3
 ; RV32-V128-NEXT:    add a0, sp, a0
 ; RV32-V128-NEXT:    addi a0, a0, 16
 ; RV32-V128-NEXT:    vs8r.v v24, (a0) # Unknown-size Folded Spill
 ; RV32-V128-NEXT:    lui a0, 699051
 ; RV32-V128-NEXT:    addi a0, a0, -1366

 ; RV64-V128-LABEL: interleave_v32i32:
 ; RV64-V128:       # %bb.0:
 ; RV64-V128-NEXT:    addi sp, sp, -16
 ; RV64-V128-NEXT:    .cfi_def_cfa_offset 16
 ; RV64-V128-NEXT:    csrr a0, vlenb
 ; RV64-V128-NEXT:    slli a0, a0, 4
 ; RV64-V128-NEXT:    sub sp, sp, a0
 ; RV64-V128-NEXT:    .cfi_escape 0x0f, 0x0d, 0x72, 0x00, 0x11, 0x10, 0x22, 0x11, 0x10, 0x92, 0xa2, 0x38, 0x00, 0x1e, 0x22 # sp + 16 + 16 * vlenb
-; RV64-V128-NEXT:    lui a0, %hi(.LCPI15_0)
-; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI15_0)
+; RV64-V128-NEXT:    lui a0, %hi(.LCPI17_0)
+; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI17_0)
 ; RV64-V128-NEXT:    li a1, 32
 ; RV64-V128-NEXT:    vsetvli zero, a1, e32, m8, ta, ma
 ; RV64-V128-NEXT:    vle32.v v0, (a0)
 ; RV64-V128-NEXT:    vmv8r.v v24, v8
 ; RV64-V128-NEXT:    vrgather.vv v8, v24, v0
 ; RV64-V128-NEXT:    addi a0, sp, 16
 ; RV64-V128-NEXT:    vs8r.v v24, (a0) # Unknown-size Folded Spill
-; RV64-V128-NEXT:    lui a0, %hi(.LCPI15_1)
-; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI15_1)
+; RV64-V128-NEXT:    lui a0, %hi(.LCPI17_1)
+; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI17_1)
 ; RV64-V128-NEXT:    vle32.v v24, (a0)
 ; RV64-V128-NEXT:    csrr a0, vlenb
 ; RV64-V128-NEXT:    slli a0, a0, 3
 ; RV64-V128-NEXT:    add a0, sp, a0
 ; RV64-V128-NEXT:    addi a0, a0, 16
 ; RV64-V128-NEXT:    vs8r.v v24, (a0) # Unknown-size Folded Spill
 ; RV64-V128-NEXT:    lui a0, 699051
 ; RV64-V128-NEXT:    addiw a0, a0, -1366

 ; V512-NEXT:    li a0, -1
 ; V512-NEXT:    vwmaccu.vx v9, a0, v10
 ; V512-NEXT:    vmv1r.v v8, v9
 ; V512-NEXT:    ret
   %a = shufflevector <4 x i8> %x, <4 x i8> poison, <4 x i32> <i32 0, i32 2, i32 1, i32 3>
   ret <4 x i8> %a
 }
 
+; This shouldn't be interleaved
+define <4 x i8> @unary_interleave_v4i8_invalid(<4 x i8> %x) {
+; V128-LABEL: unary_interleave_v4i8_invalid:
+; V128:       # %bb.0:
+; V128-NEXT:    lui a0, %hi(.LCPI19_0)
+; V128-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; V128-NEXT:    vsetivli zero, 4, e8, mf4, ta, ma
+; V128-NEXT:    vle8.v v10, (a0)
+; V128-NEXT:    vrgather.vv v9, v8, v10
+; V128-NEXT:    vmv1r.v v8, v9
+; V128-NEXT:    ret
+;
+; V512-LABEL: unary_interleave_v4i8_invalid:
+; V512:       # %bb.0:
+; V512-NEXT:    lui a0, %hi(.LCPI19_0)
+; V512-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; V512-NEXT:    vsetivli zero, 4, e8, mf8, ta, ma
+; V512-NEXT:    vle8.v v10, (a0)
+; V512-NEXT:    vrgather.vv v9, v8, v10
+; V512-NEXT:    vmv1r.v v8, v9
+; V512-NEXT:    ret
+  %a = shufflevector <4 x i8> %x, <4 x i8> poison, <4 x i32> <i32 0, i32 3, i32 1, i32 4>
+  ret <4 x i8> %a
+}
+
 define <4 x i16> @unary_interleave_v4i16(<4 x i16> %x) {
 ; V128-LABEL: unary_interleave_v4i16:
 ; V128:       # %bb.0:
 ; V128-NEXT:    vsetivli zero, 2, e16, mf2, ta, ma
 ; V128-NEXT:    vslidedown.vi v10, v8, 2
 ; V128-NEXT:    vsetivli zero, 2, e16, mf4, ta, ma
 ; V128-NEXT:    vwaddu.vv v9, v8, v10
 ; V128-NEXT:    li a0, -1
; V512-NEXT:    ret
   %a = shufflevector <4 x i32> %x, <4 x i32> poison, <4 x i32> <i32 0, i32 2, i32 1, i32 3>
   ret <4 x i32> %a
 }
 
 ; FIXME: Is there better codegen we can do here?
 define <4 x i64> @unary_interleave_v4i64(<4 x i64> %x) {
 ; RV32-V128-LABEL: unary_interleave_v4i64:
 ; RV32-V128:       # %bb.0:
-; RV32-V128-NEXT:    lui a0, %hi(.LCPI19_0)
-; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; RV32-V128-NEXT:    lui a0, %hi(.LCPI22_0)
+; RV32-V128-NEXT:    addi a0, a0, %lo(.LCPI22_0)
 ; RV32-V128-NEXT:    vsetivli zero, 4, e64, m2, ta, ma
 ; RV32-V128-NEXT:    vle16.v v12, (a0)
 ; RV32-V128-NEXT:    vrgatherei16.vv v10, v8, v12
 ; RV32-V128-NEXT:    vmv.v.v v8, v10
 ; RV32-V128-NEXT:    ret
 ;
 ; RV64-V128-LABEL: unary_interleave_v4i64:
 ; RV64-V128:       # %bb.0:
-; RV64-V128-NEXT:    lui a0, %hi(.LCPI19_0)
-; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; RV64-V128-NEXT:    lui a0, %hi(.LCPI22_0)
+; RV64-V128-NEXT:    addi a0, a0, %lo(.LCPI22_0)
 ; RV64-V128-NEXT:    vsetivli zero, 4, e64, m2, ta, ma
 ; RV64-V128-NEXT:    vle64.v v12, (a0)
 ; RV64-V128-NEXT:    vrgather.vv v10, v8, v12
 ; RV64-V128-NEXT:    vmv.v.v v8, v10
 ; RV64-V128-NEXT:    ret
 ;
 ; RV32-V512-LABEL: unary_interleave_v4i64:
 ; RV32-V512:       # %bb.0:
-; RV32-V512-NEXT:    lui a0, %hi(.LCPI19_0)
-; RV32-V512-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; RV32-V512-NEXT:    lui a0, %hi(.LCPI22_0)
+; RV32-V512-NEXT:    addi a0, a0, %lo(.LCPI22_0)
 ; RV32-V512-NEXT:    vsetivli zero, 4, e64, m1, ta, ma
 ; RV32-V512-NEXT:    vle16.v v10, (a0)
 ; RV32-V512-NEXT:    vrgatherei16.vv v9, v8, v10
 ; RV32-V512-NEXT:    vmv.v.v v8, v9
 ; RV32-V512-NEXT:    ret
 ;
 ; RV64-V512-LABEL: unary_interleave_v4i64:
 ; RV64-V512:       # %bb.0:
-; RV64-V512-NEXT:    lui a0, %hi(.LCPI19_0)
-; RV64-V512-NEXT:    addi a0, a0, %lo(.LCPI19_0)
+; RV64-V512-NEXT:    lui a0, %hi(.LCPI22_0)
+; RV64-V512-NEXT:    addi a0, a0, %lo(.LCPI22_0)
 ; RV64-V512-NEXT:    vsetivli zero, 4, e64, m1, ta, ma
 ; RV64-V512-NEXT:    vle64.v v10, (a0)
 ; RV64-V512-NEXT:    vrgather.vv v9, v8, v10
 ; RV64-V512-NEXT:    vmv.v.v v8, v9
 ; RV64-V512-NEXT:    ret
   %a = shufflevector <4 x i64> %x, <4 x i64> poison, <4 x i32> <i32 0, i32 2, i32 1, i32 3>
   ret <4 x i64> %a
 }