This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorize] Clear cache of `LoopAccessInfoManager`
ClosedPublic

Authored by Saldivarcher on Dec 7 2022, 7:39 PM.

Details

Summary

LAI is cached during the LoopDistribute pass, and is later re-used during LoopVectorize. The problem is that LoopVectorize changes SCEV, and the cached LAI does not get updated. Hence, when re-using the cached LAI, it references an invalid SCEV.

Fixes #59319

Diff Detail

Event Timeline

Saldivarcher created this revision.Dec 7 2022, 7:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2022, 7:39 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
Saldivarcher requested review of this revision.Dec 7 2022, 7:39 PM
fhahn added a reviewer: fhahn.Dec 8 2022, 1:58 AM
fhahn added a subscriber: fhahn.

Thanks for the patch! Some comments inline.

Could you add Fixes #59319 to the description, to make sure the issue is closed once this lands?

llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

We only need to clear if changes were made AFAICT

llvm/test/Transforms/LoopDistribute/pr59319.ll
3 ↗(On Diff #481142)

nit: remove

5 ↗(On Diff #481142)

Is this needed? If so, the test requires something like ; REQUIRES: x86-registered-target to make sure it is only run when the X86 backend is built.

13 ↗(On Diff #481142)

nit: strip the indvars. prefix, it doesn't add anything and makes the names unnecessarily long.

24 ↗(On Diff #481142)

Could you improve the naming of the blocks, e.g. loop.1, loop.2, something like that to make things easier to parse.

Saldivarcher edited the summary of this revision. (Show Details)Dec 8 2022, 8:09 AM

@fhahn thanks for taking a look! I made all the changes/improvements you suggested, looks a lot better.

Yeah, the assertion is only hit when a target is present, so that's why the target was there.

Saldivarcher added inline comments.Dec 8 2022, 9:18 AM
llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

The assertion is still hit if we only clear when changes are made, I think it's because we insert the loop here, then fail right after. So, there's still cached data when there are no changes made. Keep in mind that the LoopDistribute pass fails with all the loops, and all fail here.

Saldivarcher marked 4 inline comments as done.Dec 8 2022, 2:04 PM
vkmr added a subscriber: vkmr.Dec 9 2022, 5:07 PM

@fhahn did you have any other comments/concerns?

fhahn requested changes to this revision.Dec 14 2022, 6:42 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

Right, this indicates that LoopDistribute isn't the right place to fix it. We should only need invalidation *after* the IR changes. If the IR stays the same, there should be no issue with the cached loop-info objects.

I think the right place to invalidate here is in LoopVectorize's runImpl.

llvm/test/Transforms/LoopDistribute/pr59319.ll
2 ↗(On Diff #481321)

no need to use <

5 ↗(On Diff #481321)

nit: usually there's a space after ; both here and below.

8 ↗(On Diff #481321)

check the full IR?

This revision now requires changes to proceed.Dec 14 2022, 6:42 AM
fhahn added inline comments.Dec 14 2022, 6:42 AM
llvm/test/Transforms/LoopDistribute/pr59319.ll
39 ↗(On Diff #481321)

move the end of function

Saldivarcher added inline comments.Dec 16 2022, 2:28 PM
llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

When you mean invalidate, do you mean to clear the cache within LoopAccessInfoMap?

Saldivarcher marked an inline comment as not done.Dec 16 2022, 2:29 PM
Saldivarcher added inline comments.
llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

When you mean invalidate, do you mean to clear the cache within LoopAccessInfoMap?

When you say*

fhahn added inline comments.Dec 21 2022, 9:42 AM
llvm/lib/Transforms/Scalar/LoopDistribute.cpp
995 ↗(On Diff #481142)

When you mean invalidate, do you mean to clear the cache within LoopAccessInfoMap?

Yes

Saldivarcher marked an inline comment as not done.
Saldivarcher retitled this revision from [LoopDistribute] Clear cache of `LoopAccessInfoManager` to [LoopVectorize] Clear cache of `LoopAccessInfoManager`.
llvm/test/Transforms/LoopVectorize/pr59319.ll
9 ↗(On Diff #486153)

check the full IR?

The resulting IR is pretty long, around ~150 lines. I think it might be a bit exhaustive to check the full resulting IR, is there something else I should check?

Saldivarcher added inline comments.Jan 3 2023, 8:05 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10609

@fhahn I wasn't sure where the best place to invalidate cache, if you think I should do it elsewhere I can do that.

Saldivarcher added inline comments.Jan 3 2023, 8:07 PM
llvm/test/Transforms/LoopVectorize/pr59319.ll
1 ↗(On Diff #486153)

@fhahn I moved the test over to the LoopVectorize directory, hope that's alright! :)

fhahn added inline comments.Jan 4 2023, 6:46 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10609

It *should* be sufficient to do it if processLoop at line 10632 made modifications I think

llvm/test/Transforms/LoopVectorize/pr59319.ll
5 ↗(On Diff #486153)

If it requires X86, please move it to the X86 subdirectory.

Saldivarcher added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
10609

That makes more sense, I only invalidate if something changed.

llvm/test/Transforms/LoopVectorize/X86/pr59319.ll
1 ↗(On Diff #486310)

Ok, moved to the x86 directory, thanks for pointing that out.

fhahn accepted this revision.Jan 5 2023, 2:58 PM

LGTM, with the comments addressed.

llvm/test/Transforms/LoopVectorize/X86/pr59319.ll
1 ↗(On Diff #486310)

is it possible to specify a fixed VF/UF using -force-vector-width=X -force-vector-interleave=Y?

5 ↗(On Diff #486310)

not needed now

8 ↗(On Diff #486310)

It still would be good to make sure the IR we generate makes sense. You can use llvm/utils/update_test_checks.py to generate checks.

This revision is now accepted and ready to land.Jan 5 2023, 2:58 PM
fhahn added a comment.Jan 5 2023, 2:59 PM

As #59319 describes, enabling LoopDistribute along with LoopVectorize causes an assertion to be hit. The reason seems to be because the LoopDistribute pass doesn't clear out the cache of the LoopAccessInfoManager.

This needs updating, the reason is that we cache LAI during LoopDistribute, LoopVectorize makes changes that invalidate SCEV and then we re-use the cache LAI which references invalid SCEV expressions.

Saldivarcher edited the summary of this revision. (Show Details)
Saldivarcher marked 4 inline comments as done.Jan 5 2023, 5:44 PM

As #59319 describes, enabling LoopDistribute along with LoopVectorize causes an assertion to be hit. The reason seems to be because the LoopDistribute pass doesn't clear out the cache of the LoopAccessInfoManager.

This needs updating, the reason is that we cache LAI during LoopDistribute, LoopVectorize makes changes that invalidate SCEV and then we re-use the cache LAI which references invalid SCEV expressions.

Updated!

llvm/test/Transforms/LoopVectorize/X86/pr59319.ll
8 ↗(On Diff #486310)

Addressed all the comments. TIL about update_test_checks.py, thank you!

Saldivarcher marked an inline comment as done.Jan 5 2023, 5:45 PM
fhahn accepted this revision.Jan 6 2023, 12:56 AM

LGTM, thanks! Just a few additional small comments for the test .

llvm/test/Transforms/LoopVectorize/X86/pr59319.ll
5 ↗(On Diff #486724)

I *think* with the vectorization factor forced, the test doesn't need to be X86 specific. I think the triple now could be removed and the test moved out of the X86 directory.

might also be good to include a quick comment in the test that this tests LoopAccessInfo invalidation after LV modifying the IR. Might also be good to include something like loop-access-info-invalidation in the test name (in addition to prXXXX)

LGTM, thanks! Just a few additional small comments for the test .

Thanks for taking the time to review this stuff, I appreciate it!

All comments should be addressed now. :)

@fhahn also, would you be able to commit this for me? I don't have commit access.

This revision was landed with ongoing or failed builds.Jan 11 2023, 1:04 AM
This revision was automatically updated to reflect the committed changes.