Fix memcheck interval ends for pointers with negative strides

[sbaranga]

Summary

The checking pointer grouping algorithm assumes that the
starts/ends of the pointers are well formed (start <= end).

The runtime memory checking algorithm also assumes this by doing:

start0 < end1 && start1 < end0

to detect conflicts. This check only works if start0 <= end0 and
start1 <= end1.

This change correctly orders the interval ends by either checking
the stride (if it is constant) or by using min/max SCEV expressions.

[anemet]

Is this a preexisting bug to grouping pointers? IOW, did we handle swapping start and end like this for pointers before your change?

If this was a bug even before, we probably want to benchmark this to avoid surprises. (In theory we could have had a case where the memchecks should have failed but did not but do after your change.)

Please also add a test for the non-constant case.

Thanks!

lib/Analysis/LoopAccessAnalysis.cpp

144–147

Nit: you can assign to ScStart and ScEnd directly.

[anemet]

Forgot to add that LGTM :)

[sbaranga]

In D11149#204727, @anemet wrote:

Is this a preexisting bug to grouping pointers? IOW, did we handle swapping start and end like this for pointers before your change?

Yes, this was a pre-existing bug. Now the pointer grouping would just make it more difficult for someone to debug if they would hit this issue (besides also relying on this).

If this was a bug even before, we probably want to benchmark this to avoid surprises. (In theory we could have had a case where the memchecks should have failed but did not but do after your change.)

Correct. I'm running benchmarks now. Hopefully we won't get any regressions.

Please also add a test for the non-constant case.

Thanks!

Thanks,
Silviu

[sbaranga]

Rebase and add a test for the non-constant stride case.

[rengolin]

Nice catch! LGTM too.

Have we regressed anything? I seriously doubt it, but it's always good to check.

cheers,
--renato

[sbaranga]

Hi Renato,

In D11149#206320, @rengolin wrote:

Nice catch! LGTM too.

Have we regressed anything? I seriously doubt it, but it's always good to check.

cheers,
--renato

Thanks! I've tested with the llvm test suite, spec2000 and spec2006 for aarch64. I didn't see any regressions. I've seen some potential improvements in some spec benchmarks, but I think it is only noise.

-Silviu

lib/Analysis/LoopAccessAnalysis.cpp

144–147

Looks like I've missed this comment, will fix before committing.

[sbaranga]

Remove the temporaries Min and Max, since we can do the operation without using them.

[sbaranga]

Committed in r242400!

[anemet]

In D11149#206328, @sbaranga wrote:

> Thanks! I've tested with the llvm test suite, spec2000 and spec2006 for aarch64. I didn't see any regressions. I've seen some potential improvements in some spec benchmarks, but I think it is only noise.

Great, thanks for your work.

[klimek]

Test for bouncing, ignore.

lib/Analysis/LoopAccessAnalysis.cpp

 void RuntimePointerChecking::insert(Loop *Lp, Value *Ptr, bool WritePtr,
                                     unsigned DepSetId, unsigned ASId,
                                     const ValueToValueMap &Strides) {
   // Get the stride replaced scev.
   const SCEV *Sc = replaceSymbolicStrideSCEV(SE, Strides, Ptr);
   const SCEVAddRecExpr *AR = dyn_cast<SCEVAddRecExpr>(Sc);
   assert(AR && "Invalid addrec expression");
   const SCEV *Ex = SE->getBackedgeTakenCount(Lp);
+
+  const SCEV *ScStart = AR->getStart();
   const SCEV *ScEnd = AR->evaluateAtIteration(Ex, *SE);
-  Pointers.emplace_back(Ptr, AR->getStart(), ScEnd, WritePtr, DepSetId, ASId,
-                        Sc);
+  const SCEV *Step = AR->getStepRecurrence(*SE);
+
+  // For expressions with negative step, the upper bound is ScStart and the
+  // lower bound is ScEnd.
+  if (const SCEVConstant *CStep = dyn_cast<const SCEVConstant>(Step)) {
+    if (CStep->getValue()->isNegative())
+      std::swap(ScStart, ScEnd);
+  } else {
+    // Fallback case: the step is not constant, but the 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);
+  }
+
+  Pointers.emplace_back(Ptr, ScStart, ScEnd, WritePtr, DepSetId, ASId, Sc);
 }
 
 bool RuntimePointerChecking::needsChecking(
     const CheckingPtrGroup &M, const CheckingPtrGroup &N,
     const SmallVectorImpl<int> *PtrPartition) const {
   for (unsigned I = 0, EI = M.Members.size(); EI != I; ++I)
     for (unsigned J = 0, EJ = N.Members.size(); EJ != J; ++J)
       if (needsChecking(M.Members[I], N.Members[J], PtrPartition))

test/Analysis/LoopAccessAnalysis/reverse-memcheck-bounds.ll

+; RUN: opt -loop-accesses -analyze < %s | FileCheck %s
+
+; The runtime memory check code and the access grouping
+; algorithm both assume that the start and end values
+; for an access range are ordered (start <= stop).
+; When generating checks for accesses with negative stride
+; we need to take this into account and swap the interval
+; ends.
+;
+;   for (i = 0; i < 10000; i++) {
+;     B[i] = A[15000 - i] * 3;
+;   }
+
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64--linux-gnueabi"
+
+; CHECK: function 'f':
+; CHECK: (Low: (20000 + %a) High: (60000 + %a))
+
+@B = common global i32* null, align 8
+@A = common global i32* null, align 8
+
+define void @f() {
+entry:
+  %a = load i32*, i32** @A, align 8
+  %b = load i32*, i32** @B, align 8
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %idx = phi i64 [ 0, %entry ], [ %add, %for.body ]
+  %negidx = sub i64 15000, %idx
+
+  %arrayidxA0 = getelementptr inbounds i32, i32* %a, i64 %negidx
+  %loadA0 = load i32, i32* %arrayidxA0, align 2
+
+  %res = mul i32 %loadA0, 3
+
+  %add = add nuw nsw i64 %idx, 1
+
+  %arrayidxB = getelementptr inbounds i32, i32* %b, i64 %idx
+  store i32 %res, i32* %arrayidxB, align 2
+
+  %exitcond = icmp eq i64 %idx, 10000
+  br i1 %exitcond, label %for.end, label %for.body
+
+for.end:                                          ; preds = %for.body
+  ret void
+}
+
+; CHECK: function 'g':
+; When the stride is not constant, we are forced to do umin/umax to get
+; the interval limits.
+
+;   for (i = 0; i < 10000; i++) {
+;     B[i] = A[15000 - step * i] * 3;
+;   }
+
+; Here it is not obvious what the limits are, since 'step' could be negative.
+
+; CHECK: Low: (-1 + (-1 * ((-60001 + (-1 * %a)) umax (-60001 + (40000 * %step) + (-1 * %a)))))
+; CHECK: High: ((60000 + %a) umax (60000 + (-40000 * %step) + %a))
+
+define void @g(i64 %step) {
+entry:
+  %a = load i32*, i32** @A, align 8
+  %b = load i32*, i32** @B, align 8
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %idx = phi i64 [ 0, %entry ], [ %add, %for.body ]
+  %idx_mul = mul i64 %idx, %step
+  %negidx = sub i64 15000, %idx_mul
+
+  %arrayidxA0 = getelementptr inbounds i32, i32* %a, i64 %negidx
+  %loadA0 = load i32, i32* %arrayidxA0, align 2
+
+  %res = mul i32 %loadA0, 3
+
+  %add = add nuw nsw i64 %idx, 1
+
+  %arrayidxB = getelementptr inbounds i32, i32* %b, i64 %idx
+  store i32 %res, i32* %arrayidxB, align 2
+
+  %exitcond = icmp eq i64 %idx, 10000
+  br i1 %exitcond, label %for.end, label %for.body
+
+for.end:                                          ; preds = %for.body
+  ret void
+}