[SVE][InstCombine] Fix TypeSize warning in canReplaceGEPIdxWithZero

[joechrisellis]

Summary

The warning would fire when calling canReplaceGEPIdxWithZero on a GEP
whose source element type is a scalable vector. The size of scalable
vector types is not known, so this optimization cannot be performed.

This patch fixes the issue by:

• bailing out early in this routine if the GEP instruction's source element type is a scalable vector.

• making use of getFixedSize -- this removes the dependency on the deprecated interface.

[joechrisellis]

Oops -- be sure to use getFixedSize.

[fpetrogalli]

LGTM! Thanks.

[sdesmalen]

llvm/test/Transforms/InstCombine/gep-can-replace-gep-idx-with-zero-typesize.ll

1

You should make the tests more specific than running with -O2 because it is not explicitly defined what passes are run at -O2 In this case you're fixing InstCombine, so the test should use opt -S -instcombine.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp

static bool canReplaceGEPIdxWithZero(InstCombinerImpl &IC,
   // type. See if the next index is not a constant.
   Idx = FirstNZIdx(GEPI);
   if (Idx == GEPI->getNumOperands())
     return false;
   if (isa<Constant>(GEPI->getOperand(Idx)))
     return false;
 
   SmallVector<Value *, 4> Ops(GEPI->idx_begin(), GEPI->idx_begin() + Idx);
-  Type *AllocTy =
-    GetElementPtrInst::getIndexedType(GEPI->getSourceElementType(), Ops);
+  Type *SourceElementType = GEPI->getSourceElementType();
+  // Size information about scalable vectors is not available, so we cannot
+  // deduce whether indexing at n is undefined behaviour or not. Bail out.
+  if (isa<ScalableVectorType>(SourceElementType))
+    return false;
+
+  Type *AllocTy = GetElementPtrInst::getIndexedType(SourceElementType, Ops);
   if (!AllocTy || !AllocTy->isSized())
     return false;
   const DataLayout &DL = IC.getDataLayout();
-  uint64_t TyAllocSize = DL.getTypeAllocSize(AllocTy);
+  uint64_t TyAllocSize = DL.getTypeAllocSize(AllocTy).getFixedSize();
 
   // If there are more indices after the one we might replace with a zero, make
   // sure they're all non-negative. If any of them are negative, the overall
   // address being computed might be before the base address determined by the
   // first non-zero index.
   auto IsAllNonNegative = [&]() {
     for (unsigned i = Idx+1, e = GEPI->getNumOperands(); i != e; ++i) {
       KnownBits Known = IC.computeKnownBits(GEPI->getOperand(i), 0, MemI);

llvm/test/Transforms/InstCombine/gep-can-replace-gep-idx-with-zero-typesize.ll

+; RUN: opt -S -O2 -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This regression test is verifying that the optimization defined by
+; canReplaceGEPIdxWithZero, which replaces a GEP index with zero iff we can show
+; a value other than zero would cause undefined behaviour, does not throw a
+; 'assumption that TypeSize is not scalable' warning when the source element type
+; is a scalable vector.
+
+; If the source element is a scalable vector type, then we cannot deduce whether
+; or not indexing at a given index is undefined behaviour, because the size of
+; the vector is not known.
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions
+; on how to resolve it.
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+declare void @do_something(<vscale x 4 x i32> %x)
+
+define void @can_replace_gep_idx_with_zero_typesize(i64 %n, <vscale x 4 x i32>* %a, i64 %b) {
+  %idx = getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* %a, i64 %b
+  %tmp = load <vscale x 4 x i32>, <vscale x 4 x i32>* %idx
+  call void @do_something(<vscale x 4 x i32> %tmp)
+  ret void
+}