[LoopUtils] Fix incorrect RT check bounds for loop-invariant mem accesses

[mdchen]

Summary

void m(int *p, int *p2, int q) {
  int *a = p;
  int i, j, r;
  i = 0;
  for (; i < 63; i++) {
    r = j = 0;
    for (; j < 63; j++)
      r -= p2[j];
    a[i] = r;
  }
}

For the code above, expandBounds that works for LoopVectorize can
generate incorrect runtime checks that the range generated for the p2
group is [p2+62, p2+63) rather than [p2, p2+63).

https://bugs.llvm.org/show_bug.cgi?id=50686

[mdchen]

Added a test case.

[fhahn]

Thanks for the patch. Some comments inline

llvm/lib/Transforms/Utils/LoopUtils.cpp

1598

Can we instead directly check if CG->Low == Sc and CG->High == SC + 1, which is the assumption for the check that is generated?

As an additional test case, could you add a case where we have 2 invariant members with the same addresses, if that's possible?

llvm/test/Transforms/LoopVectorize/pr50686.ll

4 ↗

(On Diff #351781)

is this needed?

91 ↗

(On Diff #351781)

unneeded? Might be good to reduce/clean-up the IR a bit.

105 ↗

(On Diff #351781)

is this needed?

[mdchen]

llvm/lib/Transforms/Utils/LoopUtils.cpp

1598

Thanks! Have added a test for invariant members with the same addresses. But the check should be CG->Low == CG->High IIUC.

[reames]

Not familiar with this code.

[mdchen]

llvm/test/Transforms/LoopVectorize/pr50686.ll

19 ↗

(On Diff #352129)

Ahh..I think the range is still incorrect now, which should be [0, 3) here rather than [0, 2).

[mdchen]

Updated previous failed x86 cases.

As a explanation for the changes, I think it's unnecessary to deal with loop-invariant cases separately
in expandBounds. Instead, we should assign the correct left closed right open range to invariant
accesses in RuntimePointerChecking::insert. Please correct me if I missed something, thanks!

[mdchen]

Ping

[mdchen]

ping : )

[fhahn]

Thanks for the update. Makes sense to me, it would just be good to reduce the noise in the test diffs

llvm/test/Transforms/LoopVectorize/invariant-store-vectorization.ll

366 ↗

(On Diff #353265)

It’s hard to tell if there are actual IR changes in the test. Could you make sure only actual changes to the IR are included in the diff and try to avoid the noise from various name changes in the auto generated tests?

Same for some of the other tests

[mdchen]

@fhahn Thanks for your advice. I modified these related tests in D105438. After that is merged, I'll update this revision with one looks cleaner.

[fhahn]

In D104148#2858263, @mdchen wrote:

@fhahn Thanks for your advice. I modified these related tests in D105438. After that is merged, I'll update this revision with one looks cleaner.

Great thanks! You could probably already rebase the patch on top of D105438 and upload the new version.

[mdchen]

Updated the tests. This needs to be merged after D105438.

[mdchen]

Updated.

[fhahn]

llvm/lib/Analysis/LoopAccessAnalysis.cpp

204 ↗

(On Diff #356944)

I think we should use the same code as in the else part to compute ScEnd. Perhaps we can just keep ScStart = ScEnd = Sc and move the increment code out of the else.

// Add the size of the pointed element to ScEnd.
auto &DL = Lp->getHeader()->getModule()->getDataLayout();
Type *IdxTy = DL.getIndexType(Ptr->getType());
const SCEV *EltSizeSCEV =
    SE->getStoreSizeOfExpr(IdxTy, Ptr->getType()->getPointerElementType());
ScEnd = SE->getAddExpr(ScEnd, EltSizeSCEV);

[mdchen]

Address @fhahn's comments.

[fhahn]

LGTM, thanks! It would be good to clean up llvm/test/Transforms/LoopVectorize/pr50686.ll a bit as well, to make maintaining it easier in the future.

llvm/test/Transforms/LoopVectorize/pr50686.ll

94 ↗

(On Diff #357542)

Could you make the input IR a bit more compact by removing unnecessary BBs, potentially unnecessary instructions and adjust the names of the basic blocks so it is easier to spot the loops.

%for.cond5.preheader is misleadingly named for example, as it is *not* a pre-header.

This will make our live easier if the test needs updating in the future;

110 ↗

(On Diff #357542)

are all memory accesses needed here?

118 ↗

(On Diff #357542)

are all memory accesses here needed to reproduce the issue?

[mdchen]

Address @fhahn 's suggestions.

[fhahn]

Thanks, still LGTM!

[mdchen]

Rebase.

llvm/lib/Transforms/Utils/LoopUtils.cpp

static PointerBounds expandBounds(const RuntimeCheckingPtrGroup *CG,
   const SCEV *Sc = SE->getSCEV(Ptr);
 
   unsigned AS = Ptr->getType()->getPointerAddressSpace();
   LLVMContext &Ctx = Loc->getContext();
 
   // Use this type for pointer arithmetic.
   Type *PtrArithTy = Type::getInt8PtrTy(Ctx, AS);
 
-  if (SE->isLoopInvariant(Sc, TheLoop)) {
+  if (SE->isLoopInvariant(Sc, TheLoop) && CG->Members.size() == 1) {
     LLVM_DEBUG(dbgs() << "LAA: Adding RT check for a loop invariant ptr:"
                       << *Ptr << "\n");
     // Ptr could be in the loop body. If so, expand a new one at the correct
     // location.
     Instruction *Inst = dyn_cast<Instruction>(Ptr);
     Value *NewPtr = (Inst && TheLoop->contains(Inst))
                         ? Exp.expandCodeFor(Sc, PtrArithTy, Loc)
                         : Ptr;