This is an archive of the discontinued LLVM Phabricator instance.

[DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151).
ClosedPublic

Authored by ebrevnov on Jan 20 2020, 3:45 AM.

Details

Summary

This is second attempt to fix the problem with incorrect dependencies reported in presence of invariant load. Initial fix (https://reviews.llvm.org/D64405) was reverted due to a regression reported in https://reviews.llvm.org/D70516.

The original fix changed caching behavior for invariant loads. Namely such loads are not put into the second level cache (NonLocalDepInfo). The problem with that fix is the first level cache (CachedNonLocalPointerInfo) still works as if invariant loads were in the second level cache. The solution is in addition to not putting dependence results into the second level cache avoid putting info about invariant loads into the first level cache as well.

Diff Detail

Event Timeline

ebrevnov created this revision.Jan 20 2020, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 3:45 AM
ebrevnov retitled this revision from [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151). to [WIP][DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151)..Jan 20 2020, 4:07 AM
ebrevnov edited the summary of this revision. (Show Details)
ebrevnov edited the summary of this revision. (Show Details)Jan 20 2020, 4:19 AM
ebrevnov updated this revision to Diff 239245.Jan 21 2020, 12:51 AM

Added regression test.

ebrevnov retitled this revision from [WIP][DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151). to [DependenceAnalysis] Dependecies for loads marked with "ivnariant.load" should not be shared with general accesses(PR42151)..Jan 21 2020, 12:55 AM
ebrevnov added reviewers: jdoerfert, reames.

Just to make sure, this contains a test for the regression that was reported and lead to the initial revert, correct?

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
1395

Why is the foundBlock stuff gone?

ebrevnov marked an inline comment as done.Jan 21 2020, 8:59 PM

Just to make sure, this contains a test for the regression that was reported and lead to the initial revert, correct?

Yes. It's @test4.

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
1395

This is change was done in the previous version of the patch. The reason for that was invariant loads which are not expected to be found in the cache. Now this whole cycle is guarded with !isInvariantLoad and we can restore previous code.

jdoerfert added inline comments.Jan 22 2020, 8:27 AM
llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
1395

Let's do that then.

ebrevnov updated this revision to Diff 240588.Jan 27 2020, 7:50 AM

Updated as agreed.

This revision is now accepted and ready to land.Feb 13 2020, 6:49 AM
This revision was automatically updated to reflect the committed changes.