InterleaveAccessPass: Avoid constructing invalid shuffle masks

[MatzeB]

Summary

Fix a bug where we would construct shufflevector instructions addressing
invalid elements.

[mssimpso]

Hi Matthias,

This looks good to me. Nice find!

I think in the past there may have been some effort made to keep the ARM and AArch64 tests in sync, but having just scrutinized all the existing tests, I know this is not strictly the case anymore. So feel free to add an identical test for AArch64 if you like, but I don't think it's necessary.

[MatzeB]

In D29313#661751, @mssimpso wrote:

Hi Matthias,

This looks good to me. Nice find!

I think in the past there may have been some effort made to keep the ARM and AArch64 tests in sync, but having just scrutinized all the existing tests, I know this is not strictly the case anymore. So feel free to add an identical test for AArch64 if you like, but I don't think it's necessary.

Thanks for the review. I've added the equivalent aarch64 test as well though it probably won't buy us anything as the bug was in the generic/common code.

[asbirlea]

LGTM.
I missed this case, thank you, Matthias!

llvm/trunk/lib/CodeGen/InterleavedAccessPass.cpp

 /// E.g. For a Factor of 2 (LaneLen=4): <4, 32, 5, 33, 6, 34, 7, 35>
 /// E.g. For a Factor of 3 (LaneLen=4): <4, 32, 16, 5, 33, 17, 6, 34, 18, 7, 35, 19>
 /// E.g. For a Factor of 4 (LaneLen=2): <8, 2, 12, 4, 9, 3, 13, 5>
 ///
 /// The particular case of an RE-interleave mask is:
 /// I.e. <0, LaneLen, ... , LaneLen*(Factor - 1), 1, LaneLen + 1, ...>
 /// E.g. For a Factor of 2 (LaneLen=4): <0, 4, 1, 5, 2, 6, 3, 7>
 static bool isReInterleaveMask(ArrayRef<int> Mask, unsigned &Factor,
-                               unsigned MaxFactor) {
+                               unsigned MaxFactor, unsigned OpNumElts) {
   unsigned NumElts = Mask.size();
   if (NumElts < 4)
     return false;
 
   // Check potential Factors.
   for (Factor = 2; Factor <= MaxFactor; Factor++) {
     if (NumElts % Factor)
       continue;
for (; I < Factor; I++) {
       } else if (SavedNoUndefs > 0) {
         // StartMask defined by some non-zero value in the j loop
         StartMask = SavedLaneValue - (LaneLen - 1 - SavedNoUndefs);
       }
       // else StartMask remains set to 0, i.e. all elements are undefs
 
       if (StartMask < 0)
         break;
+      // We must stay within the vectors; This case can happen with undefs.
+      if (StartMask + LaneLen > OpNumElts*2)
+        break;
     }
 
     // Found an interleaved mask of current factor.
     if (I == Factor)
       return true;
   }
 
   return false;
if (!SI->isSimple())
     return false;
 
   ShuffleVectorInst *SVI = dyn_cast<ShuffleVectorInst>(SI->getValueOperand());
   if (!SVI || !SVI->hasOneUse())
     return false;
 
   // Check if the shufflevector is RE-interleave shuffle.
   unsigned Factor;
-  if (!isReInterleaveMask(SVI->getShuffleMask(), Factor, MaxFactor))
+  unsigned OpNumElts = SVI->getOperand(0)->getType()->getVectorNumElements();
+  if (!isReInterleaveMask(SVI->getShuffleMask(), Factor, MaxFactor, OpNumElts))
     return false;
 
   DEBUG(dbgs() << "IA: Found an interleaved store: " << *SI << "\n");
 
   // Try to create target specific intrinsics to replace the store and shuffle.
   if (!TLI->lowerInterleavedStore(SI, SVI, Factor))
     return false;
 

llvm/trunk/test/Transforms/InterleavedAccess/AArch64/interleaved-accesses.ll

 ; NO_NEON-LABEL: @store_general_mask_factor3_negativestart(
 ; NO_NEON-NOT:     @llvm.aarch64.neon
 ; NO_NEON:         ret void
 ;
   %interleaved.vec = shufflevector <32 x i32> %v0, <32 x i32> %v1, <12 x i32> <i32 undef, i32 32, i32 16, i32 undef, i32 33, i32 17, i32 undef, i32 34, i32 18, i32 2, i32 35, i32 19>
   store <12 x i32> %interleaved.vec, <12 x i32>* %ptr, align 4
   ret void
 }
+
+@g = external global <4 x float>
+
+; The following does not give a valid interleaved store
+; NEON-LABEL: define void @no_interleave
+; NEON-NOT: call void @llvm.aarch64.neon.st2
+; NEON: shufflevector
+; NEON: store
+; NEON: ret void
+; NO_NEON-LABEL: define void @no_interleave
+; NO_NEON: shufflevector
+; NO_NEON: store
+; NO_NEON: ret void
+define void @no_interleave(<4 x float> %a0) {
+  %v0 = shufflevector <4 x float> %a0, <4 x float> %a0, <4 x i32> <i32 0, i32 3, i32 7, i32 undef>
+  store <4 x float> %v0, <4 x float>* @g, align 16
+  ret void
+}

llvm/trunk/test/Transforms/InterleavedAccess/ARM/interleaved-accesses.ll

 ; NO_NEON-LABEL: @store_general_mask_factor3_midstart_pass(
 ; NO_NEON-NOT:     @llvm.arm.neon
 ; NO_NEON:         ret void
 ;
   %interleaved.vec = shufflevector <32 x i32> %v0, <32 x i32> %v1, <12 x i32> <i32 undef, i32 32, i32 16, i32 1, i32 33, i32 17, i32 undef, i32 34, i32 18, i32 undef, i32 35, i32 19>
   store <12 x i32> %interleaved.vec, <12 x i32>* %ptr, align 4
   ret void
 }
+
+@g = external global <4 x float>
+
+; The following does not give a valid interleaved store
+; NEON-LABEL: define void @no_interleave
+; NEON-NOT: call void @llvm.arm.neon.vst2
+; NEON: shufflevector
+; NEON: store
+; NEON: ret void
+; NO_NEON-LABEL: define void @no_interleave
+; NO_NEON: shufflevector
+; NO_NEON: store
+; NO_NEON: ret void
+define void @no_interleave(<4 x float> %a0) {
+  %v0 = shufflevector <4 x float> %a0, <4 x float> %a0, <4 x i32> <i32 0, i32 7, i32 1, i32 undef>
+  store <4 x float> %v0, <4 x float>* @g, align 16
+  ret void
+}