[InstCombine] Skip scalable vectors in combineLoadToOperationType

[rovka]

Summary

Don't try to canonicalize loads to scalable vector types to loads
of integers.

This removes one assertion when trying to use a TypeSize as a parameter
to DataLayout::isLegalInteger. It does not handle the second part of the
function (which looks at bitcasts).

This patch also contains a NFC fix for Load Analysis, where a variable
initialization that would cause the same assertion is moved closer to
its use. This allows us to run the new test for InstCombine without
having to teach LocationSize to play nicely with scalable vectors.

[sdesmalen]

Thanks for this fix @rovka!

llvm/test/Transforms/InstCombine/load.ll

240

Is this just adding a missing test-case for existing code, or are you trying to test something else?

340

Does this need a CHECK line to test for non-assert builds?

[rovka]

llvm/test/Transforms/InstCombine/load.ll

240

I'm just trying to make sure we're not accidentally breaking anything for fixed-sized vectors. It's essentially the same test-case as test16, but with vectors instead of float (the code should do the same thing for any type of that size). It should work the same before and after this patch, so I could commit it separately if you think it makes more sense that way. Personally I think it fits well in the context of this patch, since we're not trying to exhaustively test every possible type.

340

Oops! Of course it does, I just forgot to re-run update_test_checks after getting it to pass. Thanks for catching it.

[rovka]

Update test checks. Also brush it up a bit so it fits better with the rest of the tests in the file.

[sdesmalen]

LGTM!

llvm/test/Transforms/InstCombine/load.ll

240

Okay that's fine, I agree we can keep the test. Thanks for clarifying!

llvm/lib/Analysis/Loads.cpp

Value *llvm::FindAvailablePtrLoadStore(Value *Ptr, Type *AccessTy,
                                        BasicBlock::iterator &ScanFrom,
                                        unsigned MaxInstsToScan,
                                        AliasAnalysis *AA, bool *IsLoadCSE,
                                        unsigned *NumScanedInst) {
   if (MaxInstsToScan == 0)
     MaxInstsToScan = ~0U;
 
   const DataLayout &DL = ScanBB->getModule()->getDataLayout();
-
-  // Try to get the store size for the type.
-  auto AccessSize = LocationSize::precise(DL.getTypeStoreSize(AccessTy));
-
   Value *StrippedPtr = Ptr->stripPointerCasts();
 
   while (ScanFrom != ScanBB->begin()) {
     // We must ignore debug info directives when counting (otherwise they
     // would affect codegen).
     Instruction *Inst = &*--ScanFrom;
     if (isa<DbgInfoIntrinsic>(Inst))
       continue;
if (LoadInst *LI = dyn_cast<LoadInst>(Inst))
         if (LI->isAtomic() < AtLeastAtomic)
           return nullptr;
 
         if (IsLoadCSE)
             *IsLoadCSE = true;
         return LI;
       }
 
+    // Try to get the store size for the type.
+    auto AccessSize = LocationSize::precise(DL.getTypeStoreSize(AccessTy));
+
     if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       Value *StorePtr = SI->getPointerOperand()->stripPointerCasts();
       // If this is a store through Ptr, the value is available!
       // (This is true even if the store is volatile or atomic, although
       // those cases are unlikely.)
       if (AreEquivalentAddressValues(StorePtr, StrippedPtr) &&
           CastInst::isBitOrNoopPointerCastable(SI->getValueOperand()->getType(),
                                                AccessTy, DL)) {

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static Instruction *combineLoadToOperationType(InstCombiner &IC, LoadInst &LI) {
 
   // Try to canonicalize loads which are only ever stored to operate over
   // integers instead of any other type. We only do this when the loaded type
   // is sized and has a size exactly the same as its store size and the store
   // size is a legal integer type.
   // Do not perform canonicalization if minmax pattern is found (to avoid
   // infinite loop).
   if (!Ty->isIntegerTy() && Ty->isSized() &&
+      !(Ty->isVectorTy() && Ty->getVectorIsScalable()) &&
       DL.isLegalInteger(DL.getTypeStoreSizeInBits(Ty)) &&
       DL.typeSizeEqualsStoreSize(Ty) &&
       !DL.isNonIntegralPointerType(Ty) &&
       !isMinMaxWithLoads(
           peekThroughBitcast(LI.getPointerOperand(), /*OneUseOnly=*/true))) {
     if (all_of(LI.users(), [&LI](User *U) {
           auto *SI = dyn_cast<StoreInst>(U);
           return SI && SI->getPointerOperand() != &LI &&

llvm/test/Transforms/InstCombine/load.ll

entry:
   %x2 = load float, float* %x.cast
   store float %x2, float* %b.cast
   %x2.cast = bitcast float %x2 to i32
   store i32 %x2.cast, i32* %c.cast
 
   ret void
 }
 
+define void @test16-vect(i8* %x, i8* %a, i8* %b, i8* %c) {
+; CHECK-LABEL: @test16-vect(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[C_CAST:%.*]] = bitcast i8* [[C:%.*]] to i32*
+; CHECK-NEXT:    [[TMP0:%.*]] = bitcast i8* [[X:%.*]] to i32*
+; CHECK-NEXT:    [[X11:%.*]] = load i32, i32* [[TMP0]], align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i8* [[A:%.*]] to i32*
+; CHECK-NEXT:    store i32 [[X11]], i32* [[TMP1]], align 4
+; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i8* [[B:%.*]] to i32*
+; CHECK-NEXT:    store i32 [[X11]], i32* [[TMP2]], align 4
+; CHECK-NEXT:    [[TMP3:%.*]] = bitcast i8* [[X]] to i32*
+; CHECK-NEXT:    [[X22:%.*]] = load i32, i32* [[TMP3]], align 4
+; CHECK-NEXT:    [[TMP4:%.*]] = bitcast i8* [[B]] to i32*
+; CHECK-NEXT:    store i32 [[X22]], i32* [[TMP4]], align 4
+; CHECK-NEXT:    store i32 [[X22]], i32* [[C_CAST]], align 4
+; CHECK-NEXT:    ret void
+;
+entry:
+  %x.cast = bitcast i8* %x to <4 x i8>*
+  %a.cast = bitcast i8* %a to <4 x i8>*
+  %b.cast = bitcast i8* %b to <4 x i8>*
+  %c.cast = bitcast i8* %c to i32*
+
+  %x1 = load <4 x i8>, <4 x i8>* %x.cast
+  store <4 x i8> %x1, <4 x i8>* %a.cast
+  store <4 x i8> %x1, <4 x i8>* %b.cast
+
+  %x2 = load <4 x i8>, <4 x i8>* %x.cast
+  store <4 x i8> %x2, <4 x i8>* %b.cast
+  %x2.cast = bitcast <4 x i8> %x2 to i32
+  store i32 %x2.cast, i32* %c.cast
+
+  ret void
+}
+
+
 ; Check that in cases similar to @test16 we don't try to rewrite a load when
 ; its only use is a store but it is used as the pointer to that store rather
 ; than the value.
 
 define void @test17(i8** %x, i8 %y) {
 ; CHECK-LABEL: @test17(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[X_LOAD:%.*]] = load i8*, i8** [[X:%.*]], align 8

 entry:
   %tmp = alloca i8*, align 8
   call void @initi8(i8** %tmp)
   %swifterror = bitcast i8** %tmp to %swift.error**
   %err.res = load %swift.error*, %swift.error** %swifterror, align 8
   store %swift.error* %err.res, %swift.error** %err, align 8
   ret void
 }
+
+; Make sure we don't canonicalize accesses to scalable vectors.
+define void @test20(<vscale x 4 x i8>* %x, <vscale x 4 x i8>* %y) {
+; CHECK-LABEL: @test20(
+; CHECK-NEXT:    [[X_LOAD:%.*]] = load <vscale x 4 x i8>, <vscale x 4 x i8>* [[X:%.*]], align 1
+; CHECK-NEXT:    store <vscale x 4 x i8> [[X_LOAD]], <vscale x 4 x i8>* [[Y:%.*]], align 1
+; CHECK-NEXT:    ret void
+;
+  %x.load = load <vscale x 4 x i8>, <vscale x 4 x i8>* %x, align 1
+  store <vscale x 4 x i8> %x.load, <vscale x 4 x i8>* %y, align 1
+  ret void
+}