[SVE] Fix TypeSize warning in RuntimePointerChecking::insert

[joechrisellis]

Summary

The TypeSize warning would occur because RuntimePointerChecking::insert
was not scalable vector aware. The fix is to use
ScalarEvolution::getSizeOfExpr to grab the size of types.

[fhahn]

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll

2

Ideally the tests in this directory would only use -loop-accesses -analyze and no transformation passes. Is that possible? Also, is -mattr=+sve needed?

If loop-load-elimination is required, the test should probably go there. Also it might be good to just have all scaleable tests in a single file in the directory>

[joechrisellis]

Address @fhahn's suggestions.

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll

2

Thanks for the pointers!

Ideally the tests in this directory would only use -loop-accesses -analyze and no transformation passes. Is that possible? Also, is -mattr=+sve needed?

Ah okay -- turns out we can just use -loop-accesses -analyze for the same result. Fixed this.

If loop-load-elimination is required, the test should probably go there.

It's not required. :)

[fhahn]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

227–229

nit: just IdxTy?

229

I think this slightly changes the behavior if store and alloc size are different, is that intentional? (because getSizeOfExpr uses alloc size).

[joechrisellis]

Address @fhahn's comments.

• IntIdxTy -> IdxTy
• Preserve existing behaviour for non-scalable types.

[joechrisellis]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

229

Excellent spot -- I have fixed this.

[fhahn]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

229

Not sure how hard it would be to come up with a test, but it would probably be good to have one

[joechrisellis]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

229

Hi @fhahn, sorry for the lag in replying here!

I tried to make a test for this but wasn't able to come up with anything that defends against the different behaviour when alloc size != store size. I think I will leave this out from the patch, if that is okay with you? Otherwise, it would be useful to hear your ideas on how we could create such a test. :)

[joechrisellis]

@fhahn Hi! Do you have any objections to landing this as-is without the additional test? Alternatively, any recommendations on how to test this? 😄

[fhahn]

In D90171#2410782, @joechrisellis wrote:

@fhahn Hi! Do you have any objections to landing this as-is without the additional test? Alternatively, any recommendations on how to test this? 😄

I think you would need to use element types with different store & alloca sizes, like i19 (see https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/IR/DataLayout.h#L446 for some examples). You should be able to adjust a test with runtime checks (e.g. llvm/test/Analysis/LoopAccessAnalysis/memcheck-off-by-one-error.ll) and update it to use a type like i19 to get such a test. If you decide to use llvm/test/Analysis/LoopAccessAnalysis/memcheck-off-by-one-error.ll, I think some of the casts in the loop body could be stripped.

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll

2

If loop-load-elimination is required, the test should probably go there.

It's not required. :)

Looks like there's another test that runs -loop-load-elim: llvm/test/Analysis/LoopAccessAnalysis/gep-induction-operand-typesize-warning.ll. Could this test also be updated?

[joechrisellis]

Add test to defend against accidentally using alloc size instead of store size.

[joechrisellis]

Thank you @fhahn, that comment was super helpful. I've opted to create a new test here because I didn't want to change the relationship between the IR and the C++ at the top of memcheck-off-by-on-error.ll. If you think this is the wrong thing to do, just let me know. 😄

[fhahn]

LGTM with the comment adjustment, thanks!

llvm/lib/Analysis/ScalarEvolution.cpp

3685

if you are touching the comment lines, could. you also re-flow the comment?

Also, this comment should be moved to the getConstant call, right? Because for scalable vectors we create a constant expression.

[joechrisellis]

Move and re-flow comment lines.

llvm/include/llvm/Analysis/ScalarEvolution.h

public:
   /// Return a SCEV for the constant 1 of a specific type.
   const SCEV *getOne(Type *Ty) { return getConstant(Ty, 1); }
 
   /// Return a SCEV for the constant -1 of a specific type.
   const SCEV *getMinusOne(Type *Ty) {
     return getConstant(Ty, -1, /*isSigned=*/true);
   }
 
-  /// Return an expression for sizeof AllocTy that is type IntTy
+  /// Return an expression for sizeof ScalableTy that is type IntTy, where
+  /// ScalableTy is a scalable vector type.
+  const SCEV *getSizeOfScalableVectorExpr(Type *IntTy,
+                                          ScalableVectorType *ScalableTy);
+
+  /// Return an expression for the alloc size of AllocTy that is type IntTy
   const SCEV *getSizeOfExpr(Type *IntTy, Type *AllocTy);
 
+  /// Return an expression for the store size of StoreTy that is type IntTy
+  const SCEV *getStoreSizeOfExpr(Type *IntTy, Type *StoreTy);
+
   /// Return an expression for offsetof on the given field with type IntTy
   const SCEV *getOffsetOfExpr(Type *IntTy, StructType *STy, unsigned FieldNo);
 
   /// Return the SCEV object corresponding to -V.
   const SCEV *getNegativeSCEV(const SCEV *V,
                               SCEV::NoWrapFlags Flags = SCEV::FlagAnyWrap);
 
   /// Return the SCEV object corresponding to ~V.

llvm/lib/Analysis/LoopAccessAnalysis.cpp

if (const auto *CStep = dyn_cast<SCEVConstant>(Step)) {
       // Fallback case: the step is not constant, but we can still
       // get the upper and lower bounds of the interval by using min/max
       // expressions.
       ScStart = SE->getUMinExpr(ScStart, ScEnd);
       ScEnd = SE->getUMaxExpr(AR->getStart(), ScEnd);
     }
     // Add the size of the pointed element to ScEnd.
     auto &DL = Lp->getHeader()->getModule()->getDataLayout();
-    unsigned EltSize =
-        DL.getTypeStoreSizeInBits(Ptr->getType()->getPointerElementType()) / 8;
-    const SCEV *EltSizeSCEV = SE->getConstant(ScEnd->getType(), EltSize);
+    Type *IdxTy = DL.getIndexType(Ptr->getType());
+    const SCEV *EltSizeSCEV =
+        SE->getStoreSizeOfExpr(IdxTy, Ptr->getType()->getPointerElementType());
     ScEnd = SE->getAddExpr(ScEnd, EltSizeSCEV);
   }
 
   Pointers.emplace_back(Ptr, ScStart, ScEnd, WritePtr, DepSetId, ASId, Sc);
 }
 
 SmallVector<RuntimePointerCheck, 4>
 RuntimePointerChecking::generateChecks() const {

llvm/lib/Analysis/ScalarEvolution.cpp

const SCEV *ScalarEvolution::getUMinExpr(const SCEV *LHS,
   SmallVector<const SCEV *, 2> Ops = { LHS, RHS };
   return getUMinExpr(Ops);
 }
 
 const SCEV *ScalarEvolution::getUMinExpr(SmallVectorImpl<const SCEV *> &Ops) {
   return getMinMaxExpr(scUMinExpr, Ops);
 }
 
-const SCEV *ScalarEvolution::getSizeOfExpr(Type *IntTy, Type *AllocTy) {
-  if (isa<ScalableVectorType>(AllocTy)) {
-    Constant *NullPtr = Constant::getNullValue(AllocTy->getPointerTo());
-    Constant *One = ConstantInt::get(IntTy, 1);
-    Constant *GEP = ConstantExpr::getGetElementPtr(AllocTy, NullPtr, One);
-    // Note that the expression we created is the final expression, we don't
-    // want to simplify it any further Also, if we call a normal getSCEV(),
-    // we'll end up in an endless recursion. So just create an SCEVUnknown.
-    return getUnknown(ConstantExpr::getPtrToInt(GEP, IntTy));
-  }
-  // We can bypass creating a target-independent
-  // constant expression and then folding it back into a ConstantInt.
-  // This is just a compile-time optimization.
+const SCEV *
+ScalarEvolution::getSizeOfScalableVectorExpr(Type *IntTy,
+                                             ScalableVectorType *ScalableTy) {
+  Constant *NullPtr = Constant::getNullValue(ScalableTy->getPointerTo());
+  Constant *GEP = ConstantExpr::getGetElementPtr(ScalableTy, NullPtr, One);
+
+const SCEV *ScalarEvolution::getSizeOfExpr(Type *IntTy, Type *AllocTy) {
+  if (auto *ScalableAllocTy = dyn_cast<ScalableVectorType>(AllocTy))
+    return getSizeOfScalableVectorExpr(IntTy, ScalableAllocTy);
+  // We can bypass creating a target-independent constant expression and then
+  // folding it back into a ConstantInt. This is just a compile-time
+  // optimization.
   return getConstant(IntTy, getDataLayout().getTypeAllocSize(AllocTy));
 }
 
+const SCEV *ScalarEvolution::getStoreSizeOfExpr(Type *IntTy, Type *StoreTy) {
+  if (auto *ScalableStoreTy = dyn_cast<ScalableVectorType>(StoreTy))
+    return getSizeOfScalableVectorExpr(IntTy, ScalableStoreTy);
+  // We can bypass creating a target-independent constant expression and then
+  // folding it back into a ConstantInt. This is just a compile-time
+  // optimization.
+  return getConstant(IntTy, getDataLayout().getTypeStoreSize(StoreTy));
+}
+
 const SCEV *ScalarEvolution::getOffsetOfExpr(Type *IntTy,
                                              StructType *STy,
                                              unsigned FieldNo) {
-  // We can bypass creating a target-independent
-  // constant expression and then folding it back into a ConstantInt.
-  // This is just a compile-time optimization.
+  // We can bypass creating a target-independent constant expression and then
+  // folding it back into a ConstantInt. This is just a compile-time
+  // optimization.
   return getConstant(
       IntTy, getDataLayout().getStructLayout(STy)->getElementOffset(FieldNo));
 }
 
 const SCEV *ScalarEvolution::getUnknown(Value *V) {
   // Don't attempt to do anything other than create a SCEVUnknown object
   // here.  createSCEV only calls getUnknown after checking for all other
   // interesting possibilities, and any other code that calls getUnknown

llvm/test/Analysis/LoopAccessAnalysis/memcheck-store-vs-alloc-size.ll

+; RUN: opt -analyze --loop-accesses %s -enable-new-pm=0 | FileCheck %s
+; RUN: opt -passes=print-access-info %s -disable-output 2>&1 | FileCheck %s
+
+; This test defends against accidentally using alloc size instead of store size when performing run-time
+; boundary check of memory accesses. The IR in this file is based on
+; llvm/test/Analysis/LoopAccessAnalysis/memcheck-off-by-one-error.ll.
+; Here, we use i19 instead of i64 because it has a different alloc size to its store size.
+
+;CHECK: function 'fastCopy':
+;CHECK: (Low: %op High: (27 + %op))
+;CHECK: (Low: %src High: (27 + %src))
+
+define void @fastCopy(i8* nocapture readonly %src, i8* nocapture %op) {
+entry:
+  br label %while.body.preheader
+
+while.body.preheader:                             ; preds = %entry
+  br label %while.body
+
+while.body:                                       ; preds = %while.body.preheader, %while.body
+  %len.addr.07 = phi i32 [ %sub, %while.body ], [ 32, %while.body.preheader ]
+  %op.addr.06 = phi i8* [ %add.ptr1, %while.body ], [ %op, %while.body.preheader ]
+  %src.addr.05 = phi i8* [ %add.ptr, %while.body ], [ %src, %while.body.preheader ]
+  %0 = bitcast i8* %src.addr.05 to i19*
+  %1 = load i19, i19* %0, align 8
+  %2 = bitcast i8* %op.addr.06 to i19*
+  store i19 %1, i19* %2, align 8
+  %add.ptr = getelementptr inbounds i8, i8* %src.addr.05, i19 8
+  %add.ptr1 = getelementptr inbounds i8, i8* %op.addr.06, i19 8
+  %sub = add nsw i32 %len.addr.07, -8
+  %cmp = icmp sgt i32 %len.addr.07, 8
+  br i1 %cmp, label %while.body, label %while.end.loopexit
+
+while.end.loopexit:                               ; preds = %while.body
+  br label %while.end
+
+while.end:                                        ; preds = %while.end.loopexit, %entry
+  ret void
+}

llvm/test/Analysis/LoopAccessAnalysis/runtime-pointer-checking-insert-typesize.ll

+; RUN: opt -loop-accesses -analyze < %s >/dev/null 2>%t
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This regression test is defending against a TypeSize warning 'assumption that
+; TypeSize is not scalable'. This warning cropped up in
+; RuntimePointerChecking::insert when performing loop load elimination because
+; this function was previously unaware of scalable types.
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+define void @runtime_pointer_checking_insert_typesize(<vscale x 4 x i32>* %a,
+                                                      <vscale x 4 x i32>* %b) {
+entry:
+  br label %loop.body
+loop.body:
+  %0 = phi i64 [ 0, %entry ], [%1, %loop.body]
+  %idx_a = getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* %a, i64 %0
+  %idx_b = getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* %b, i64 %0
+  %tmp = load <vscale x 4 x i32>, <vscale x 4 x i32>* %idx_a
+  store <vscale x 4 x i32> %tmp, <vscale x 4 x i32>* %idx_b
+  %1 = add i64 %0, 2
+  %2 = icmp eq i64 %1, 1024
+  br i1 %2, label %loop.end, label %loop.body
+loop.end:
+  ret void
+}