[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

231

nit: just IdxTy?

233

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

233

Excellent spot -- I have fixed this.

[fhahn]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

233

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

233

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

3544 ↗

(On Diff #307307)

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/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 *IntIdxTy = DL.getIndexType(Ptr->getType());
+    const SCEV *EltSizeSCEV =
+        SE->getSizeOfExpr(IntIdxTy, 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/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
+}