This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mdchen on Jun 11 2021, 1:17 PM.

Details

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

Diff Detail

Event Timeline

mdchen created this revision.Jun 11 2021, 1:17 PM
mdchen requested review of this revision.Jun 11 2021, 1:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 1:17 PM
mdchen edited the summary of this revision. (Show Details)Jun 11 2021, 1:24 PM
mdchen added reviewers: fhahn, reames, nikic.
mdchen updated this revision to Diff 351781.Jun 13 2021, 11:10 PM

Added a test case.

mdchen edited the summary of this revision. (Show Details)Jun 13 2021, 11:11 PM
fhahn added a comment.Jun 14 2021, 7:33 AM

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 updated this revision to Diff 352129.Jun 15 2021, 7:14 AM
mdchen retitled this revision from WIP: [LoopUtils] Fix incorrect runtimechecks to [LoopUtils] Fix incorrect runtime checks.
mdchen edited the summary of this revision. (Show Details)
mdchen marked 4 inline comments as done.
mdchen added inline comments.
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 resigned from this revision.Jun 15 2021, 3:46 PM

Not familiar with this code.

mdchen edited reviewers, added: anemet; removed: reames.Jun 16 2021, 12:29 AM
mdchen marked an inline comment as done.
mdchen added inline comments.Jun 16 2021, 12:39 AM
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 retitled this revision from [LoopUtils] Fix incorrect runtime checks to WIP: [LoopUtils] Fix incorrect runtime checks.Jun 18 2021, 1:55 AM
mdchen updated this revision to Diff 353194.Jun 19 2021, 7:30 AM
mdchen retitled this revision from WIP: [LoopUtils] Fix incorrect runtime checks to [LoopUtils] Fix incorrect runtime checks.
mdchen updated this revision to Diff 353265.Jun 20 2021, 8:31 PM
mdchen retitled this revision from [LoopUtils] Fix incorrect runtime checks to [LoopUtils] Fix incorrect RT check bounds for loop-invariant mem accesses.
mdchen edited the summary of this revision. (Show Details)

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!

fhahn added a comment.Jul 5 2021, 2:08 AM

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 added a comment.Jul 5 2021, 9:57 AM

@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 added a comment.Jul 5 2021, 10:00 AM

@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 this revision to Diff 356522.Jul 5 2021, 10:01 AM

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

mdchen updated this revision to Diff 356944.Jul 7 2021, 7:02 AM

Updated.

fhahn added inline comments.Jul 8 2021, 7:41 AM
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 updated this revision to Diff 357524.Jul 9 2021, 8:38 AM

Address @fhahn's comments.

mdchen updated this revision to Diff 357542.Jul 9 2021, 9:24 AM
fhahn accepted this revision.Jul 10 2021, 6:47 AM

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?

This revision is now accepted and ready to land.Jul 10 2021, 6:47 AM
mdchen updated this revision to Diff 357924.Jul 12 2021, 6:23 AM

Address @fhahn 's suggestions.

fhahn added a comment.Jul 12 2021, 1:22 PM

Thanks, still LGTM!

This revision was landed with ongoing or failed builds.Jul 19 2021, 4:39 AM
This revision was automatically updated to reflect the committed changes.