This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Do not plant SCEV checks unnecessarily
ClosedPublic

Authored by pawosm01 on Mar 27 2023, 5:55 AM.

Details

Summary

The vectorisation analysis collects strides for loop invariant
pointers, which is wrong because they are not strided. We don't
need to generate SCEV checks (which are costly performancewise)
for such pointers, we just need to do the appropriate aliasing
checks.

This patch fixes the problem by changing getStrideFromPointer()
to treat loop invariant pointers as having no stride.

Originally proposed by David Sherwood with further suggestions
from Florian Hahn.

Diff Detail

Event Timeline

pawosm01 created this revision.Mar 27 2023, 5:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2023, 5:55 AM
pawosm01 requested review of this revision.Mar 27 2023, 5:55 AM
ABataev added inline comments.Mar 27 2023, 6:39 AM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

Could you precommit the test?

5–6

Pass these as arguments of opt call.

8

The attributes can be dropped, assume

64–74

Can you try to drop metadata too?

pawosm01 updated this revision to Diff 508666.Mar 27 2023, 8:06 AM

Simplification of the test case (as per reviewer request).

pawosm01 marked 2 inline comments as done.Mar 27 2023, 8:08 AM
pawosm01 added inline comments.
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

will do soon(ish)

5–6

Actually, I see no good reason for having them. No need for limiting it to AArch64.

pawosm01 marked an inline comment as done.Mar 27 2023, 8:09 AM
pawosm01 marked an inline comment as done.Mar 27 2023, 8:18 AM
pawosm01 added inline comments.
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

Released as D146974

Not sure how it's going to fail correctly, Something weird is happening with the CI today.

pawosm01 added inline comments.Mar 27 2023, 1:12 PM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

The test case released as D146974 has failed as expected.

pawosm01 updated this revision to Diff 508999.Mar 28 2023, 7:02 AM

Test case simplified, as per the reviewer request.

ABataev added inline comments.Apr 4 2023, 7:52 AM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

Rebase to see the difference

pawosm01 updated this revision to Diff 510823.Apr 4 2023, 8:53 AM

I hope reupload of the same diff causes rebase...

Hi @pawelo12345678, it looks like something went wrong here because now there are no code changes, but also this patch still seems to be adding llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll. I think what you can do to show the test changes is to rebase this patch downstream in git based off D146974. You can then show the history by adding D146974 as a parent revision.

pawosm01 updated this revision to Diff 510831.Apr 4 2023, 9:05 AM

restored what was lost

context will return soon

BTW, is there any one-button-click way of rebasing?

pawosm01 updated this revision to Diff 510836.Apr 4 2023, 9:14 AM

the context also restored

ABataev added inline comments.Apr 4 2023, 9:23 AM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

Looks like the test was not precommitted

pawosm01 added inline comments.Apr 4 2023, 9:46 AM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

Yeah, I've never been through this scenario before. I need a guidance on how to do that.

ABataev added inline comments.Apr 4 2023, 10:34 AM
llvm/test/Transforms/LoopVectorize/vector-no-scevcheck.ll
2

You need to commit D146974 at first (after approval), then rebase this patch to see the difference in the test(s).

pawosm01 updated this revision to Diff 510862.Apr 4 2023, 11:08 AM

I'm trying to build a relation between D146974 and D146958.

pawosm01 updated this revision to Diff 510884.Apr 4 2023, 12:11 PM

following reviewer's advice.

pawosm01 marked 3 inline comments as done.Apr 4 2023, 12:12 PM
mgabka added a subscriber: mgabka.Apr 5 2023, 12:57 AM
fhahn added inline comments.Apr 23 2023, 2:33 PM
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2566 ↗(On Diff #510884)

Better use a more powerful SCEV based check that the AddRec for the ptr is for TheLoop ( check TheLoop != AR->getLoop() in getStrideFromPointer)

llvm/lib/Analysis/VectorUtils.cpp
1163 ↗(On Diff #510884)

Is this needed?

pawosm01 updated this revision to Diff 516794.Apr 25 2023, 7:06 AM
pawosm01 edited the summary of this revision. (Show Details)

Changes applied according to the reviewer's suggestion. With summary update.

pawosm01 marked an inline comment as done.Apr 25 2023, 7:07 AM
pawosm01 added inline comments.
llvm/lib/Analysis/LoopAccessAnalysis.cpp
2566 ↗(On Diff #510884)

Indeed it worked. And the other test case that you'd suggested is also passing too.

pawosm01 marked an inline comment as done.Apr 25 2023, 7:08 AM
pawosm01 marked an inline comment as done.Apr 25 2023, 7:10 AM
pawosm01 added inline comments.
llvm/lib/Analysis/VectorUtils.cpp
1163 ↗(On Diff #510884)

Apparently not.

fhahn accepted this revision.Apr 25 2023, 12:55 PM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 25 2023, 12:55 PM
This revision was landed with ongoing or failed builds.Apr 25 2023, 1:48 PM
This revision was automatically updated to reflect the committed changes.
pawosm01 marked an inline comment as done.