This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Check one use during GEP merging
AcceptedPublic

Authored by nikic on Jan 20 2023, 6:02 AM.

Details

Reviewers
spatel
RKSimon
Summary

Currently we try to combine two GEPs even if the inner one has more than one use. However, this means we can end up duplicating the index arithmetic.

This patch restricts GEP merging to one-use or constant source. Constant offsets are allowed, because these are considered essentially free.

TBH I'm not really sure whether we should do this, as it's long-standing behavior, and I'm not aware of it causing active problems. This came up as a question during D138637.

Diff Detail

Event Timeline

nikic created this revision.Jan 20 2023, 6:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 6:02 AM
nikic requested review of this revision.Jan 20 2023, 6:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 6:02 AM
spatel accepted this revision.Jan 20 2023, 11:38 AM

LGTM
Seems fine based on the test diffs, but I don't have any sense of whether it could cause regressions for real code.
It would be nice to break up visitGEPOfGEP() for readability. Maybe that gets easier after the branch and typed pointer support goes away?

This revision is now accepted and ready to land.Jan 20 2023, 11:38 AM