Page MenuHomePhabricator

[GVN] Clobber partially aliased loads.
Needs ReviewPublic

Authored by dfukalov on Jan 27 2021, 10:17 AM.

Details

Summary

Use offsets stored in AliasResult implemented in D98718.

Diff Detail

Event Timeline

dfukalov requested review of this revision.Jan 27 2021, 10:17 AM
dfukalov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2021, 10:17 AM
dfukalov planned changes to this revision.Jan 27 2021, 10:18 AM
dfukalov updated this revision to Diff 327811.Mar 3 2021, 8:33 AM

Implementation.

dfukalov retitled this revision from [WIP][GVN] Clobber partially aliased loads. to [GVN] Clobber partially aliased loads..Mar 3 2021, 8:39 AM
dfukalov edited the summary of this revision. (Show Details)
dfukalov added reviewers: nikic, asbirlea, fhahn.
dfukalov added a subscriber: fvrmatteo.
bmahjour removed a subscriber: bmahjour.Mar 3 2021, 8:40 AM
dfukalov planned changes to this revision.Mar 16 2021, 9:19 AM

The patch should be updated after converting AliasResult.

dfukalov updated this revision to Diff 335902.Wed, Apr 7, 12:26 PM

Rebased to use reworked AliasResult in D98718.

dfukalov edited the summary of this revision. (Show Details)Wed, Apr 7, 12:27 PM

Performance-wise this looks good.
Please wait to see if the other reviewers have comments on this.

dfukalov updated this revision to Diff 336377.Fri, Apr 9, 3:33 AM

Rebased after preparation changes (D98027+D98718) committed.

dfukalov updated this revision to Diff 336379.Fri, Apr 9, 3:38 AM

Removed unneeded change in MemorySSATest.cpp.

nikic added inline comments.Sun, Apr 11, 2:10 PM
llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
518

Do you have any information on why this is not a concern anymore? The comment was introduced in https://github.com/llvm/llvm-project/commit/a471751c24324e7ba6ac5c612dbedb16c644fc44, unfortunately without a test case :(

519

Say we have a load from X, then a load from PartialAlias of X without offset, then another load from X. I think after your change, we will no longer forward from the first load, because we'll return a clobber for the PartialAlias in between, even though it cannot be used.

Can you please test this situation? Maybe we should not return a clobber if no offset is available?

llvm/test/Transforms/GVN/clobber-partial-alias.ll
2

update_test_checks please.

dfukalov updated this revision to Diff 336899.Mon, Apr 12, 10:47 AM

Addressing comments.

dfukalov marked 2 inline comments as done.Mon, Apr 12, 10:59 AM
dfukalov added inline comments.
llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
518

Yes, I saw this change and because of this comment added VisitedPhiBBs check when we return PartialAlias with offset in BasicAAResult::aliasGEP.

519

Yes, you're right there is no reason to return here PartialAlias if we don't know the offset.