Page MenuHomePhabricator

[LICM] Hoist guards from non-header blocks
ClosedPublic

Authored by mkazantsev on Aug 17 2018, 4:50 AM.

Details

Summary

This patch relaxes overconservative checks on whether or not we could write memory
before we execute an instruction. This allows us to hoist guards out of loops even if they
are not in the header block.

Diff Detail

Repository
rL LLVM

Event Timeline

fedor.sergeev added inline comments.Aug 17 2018, 5:51 AM
include/llvm/Analysis/MustExecute.h
77 ↗(On Diff #161208)

'could not' does not seem to be a right verb here.

Perhaps 'does not' better meets the purpose (doesNotWriteMemoryBefore?).

lib/Analysis/MustExecute.cpp
193 ↗(On Diff #161208)

Since current usage of this interface is limited to invariant guards only it should not be a pressing problem compile-time wise.
But it is worth to at least consider our options.
Did you think about extending ICF to track this information as well?

mkazantsev planned changes to this revision.Aug 20 2018, 12:50 AM
mkazantsev added inline comments.
lib/Analysis/MustExecute.cpp
193 ↗(On Diff #161208)

Planning to make MemoryWriteTracking based on https://reviews.llvm.org/D50954

mkazantsev marked 2 inline comments as done.

Rebased on smarter version of tracking, improved algorithmic complexity.

fedor.sergeev added inline comments.Aug 29 2018, 1:39 PM
lib/Analysis/MustExecute.cpp
184 ↗(On Diff #161457)

I'm not sure fast-path is needed here, it will definitely be handled by the code below and will only be
as slow as initialization of empty Predecessors set.

And, anyway, comment is taken from somewhere else and is a bit confusing.
If you decide to leave it then it should say:
"there are no instructions in this loop before the header."

mkazantsev added inline comments.Aug 30 2018, 6:35 PM
lib/Analysis/MustExecute.cpp
184 ↗(On Diff #161457)

Set creation is just unneeded overhead. Agreed with the comment, it needs fixed. :)

Rebase, comment fix.

This revision is now accepted and ready to land.Aug 31 2018, 2:31 AM
mkazantsev updated this revision to Diff 172916.Nov 7 2018, 3:44 AM

Rebased. I will wait a bit if we will find any bugs in underlying LICM changes and if it's OK, I will merge it by the end of week.

This revision was automatically updated to reflect the committed changes.