This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Refactor handling of pointer-select in GVN pass
ClosedPublic

Authored by kachkov98 on Jan 12 2023, 8:48 AM.

Details

Summary

This patch extends Def memory dependency with support of select instructions to consistently handle pointer-select conversion.

In general it would be great to support address translation through select inst (extend PHITransAddr?) to optimize different cases when array index depends from select like: https://godbolt.org/z/TsarK6xEs (I've reported this issue in https://github.com/llvm/llvm-project/issues/58569), now we only handle select-between-pointer; this patch is preparation for this work.

Diff Detail

Event Timeline

kachkov98 created this revision.Jan 12 2023, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 8:48 AM
kachkov98 requested review of this revision.Jan 12 2023, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 8:48 AM
kachkov98 edited the summary of this revision. (Show Details)Jan 12 2023, 8:58 AM
kachkov98 added reviewers: fhahn, reames, mkazantsev, nikic.
nikic added a comment.Jan 12 2023, 9:07 AM

Could you please split out the AnalyzeLoadAvailability() refactoring to std::optional into a separate NFC patch (and base this one on top)?

Split AnalyzeLoadAvailability refactoring into separate patch: https://reviews.llvm.org/D141664

kachkov98 edited the summary of this revision. (Show Details)Jan 13 2023, 12:49 AM
nikic added a comment.Jan 13 2023, 1:22 AM

Thanks! Could you please also split off the change to findDominatingLoad()? For that change, we also need to introduce a limit on the number of instructions that get walked while finding the dominating value (100 would be consistent with MemDep), otherwise there may be issues with very large blocks. There should also be a test that shows we can now handle a case that we previously couldn't (with the dominating load being in a preceding basic block).

kachkov98 updated this revision to Diff 488967.Jan 13 2023, 5:23 AM

Split findDominatingLoad() rework into separate patch

nikic accepted this revision.Jan 16 2023, 3:00 AM

LGTM

It's a bit odd to have a non-memory instruction as the dependency, but this does seem to work out nicely in practice.

llvm/lib/Transforms/Scalar/GVN.cpp
1124

Outdated comment (End)

1134

Outdated comment (End).

This revision is now accepted and ready to land.Jan 16 2023, 3:00 AM
This revision was landed with ongoing or failed builds.Jan 16 2023, 3:13 AM
This revision was automatically updated to reflect the committed changes.
nikic added a comment.Jan 16 2023, 3:54 AM

It looks like this breaks the build on 32-bit targets, because we don't have enough alignment bits.

kachkov98 reopened this revision.Jan 16 2023, 5:56 AM

Since extending MemDepResult looks not feasable (and replacing it to tag-pointer pair also looks bad because it frequently passed by value), I think we can treat select dependency as Def. It also fits naturally into existing GVN infra, and the meaning of Def dependency is already overloaded (it can be other load/store/alloca/memory intrinsic/etc.)

This revision is now accepted and ready to land.Jan 16 2023, 5:56 AM
kachkov98 updated this revision to Diff 489524.Jan 16 2023, 5:57 AM

Treat select as Def dependency

kachkov98 requested review of this revision.Jan 16 2023, 5:59 AM
nikic accepted this revision.Jan 17 2023, 12:25 AM

LGTM

This revision is now accepted and ready to land.Jan 17 2023, 12:25 AM

(Patch description needs an update though)

kachkov98 edited the summary of this revision. (Show Details)Jan 17 2023, 12:34 AM
This revision was landed with ongoing or failed builds.Jan 17 2023, 12:34 AM
This revision was automatically updated to reflect the committed changes.