This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt] Pass DTU to removeUnreachableBlocks instead of recomputing.
ClosedPublic

Authored by fhahn on Oct 1 2019, 2:11 PM.

Details

Summary

removeUnreachableBlocks knows how to preserve the DomTree, so make use
of it instead of re-computing the DT.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Oct 1 2019, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 1 2019, 2:11 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
davide accepted this revision.Oct 1 2019, 2:17 PM

I think I wrote some of this code and it predates Kuba's incremental dominator updater. I'm fine, but I'll wait if he has opinions cc: @kuhar

This revision is now accepted and ready to land.Oct 1 2019, 2:17 PM
kuhar accepted this revision.Oct 1 2019, 2:18 PM

Looks good to me.

This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Jan 4 2020, 5:48 AM

I suspect that this change caused a major compile-time performance regression in GlobalOpt. Doesn't this essentially break the laziness of LookupDomTree() and force DT construction even if it will not be needed?

As the removeUnreachableBlocks() call is (presumably) expected to never actually do something outside of manually constructed IR examples, this seems like a bad tradeoff. Ideally we would only fetch the DT here if it exists, but threading that through is likely not worthwhile.

fhahn added a comment.Jan 4 2020, 6:07 AM

I suspect that this change caused a major compile-time performance regression in GlobalOpt. Doesn't this essentially break the laziness of LookupDomTree() and force DT construction even if it will not be needed?

Do you have a test case handy? Assuming the DT for F is already computed, LookupDomTree should be free, but looking at the preceding passes, it looks like IPSCCP does not preserve the DTs with the LegacyPassManager (When I tried I could not get a module pass to preserve individual function analysis).

Unless we can get the DTs preserved until here, it is probably best to use getAnalysisIfAvailable<DominatorTreeWrapperPass>() here instead.

nikic added a comment.Jan 4 2020, 11:39 AM

Yes, we're still on the legacy manager, so DT is not available here. I've just verified that reverting this patch does indeed reclaim a sizable part of the LLVM 10 compile-time regressions (end-to-end timings).

Yes, we're still on the legacy manager, so DT is not available here. I've just verified that reverting this patch does indeed reclaim a sizable part of the LLVM 10 compile-time regressions (end-to-end timings).

Maybe reason of https://bugs.llvm.org/show_bug.cgi?id=44408 too

fhahn added a comment.Jan 4 2020, 1:24 PM

Yes, we're still on the legacy manager, so DT is not available here. I've just verified that reverting this patch does indeed reclaim a sizable part of the LLVM 10 compile-time regressions (end-to-end timings).

Hm it seems like the legacy module pass manager does not allow using getAnalysisIfAvailable for function analysis :( I've put up D72214 with a workaround. Does this fix the regression as well? Otherwise I guess the easiest would be to just revert this patch until the legacy pass manager is gone....

nikic added a comment.Jan 5 2020, 1:16 AM

@fhahn I've verified that D72214 also fixes the issue (performance difference to a simple revert is in the noise).

fhahn added a comment.Jan 14 2020, 6:59 AM

I've revert rG192cce10f67e for now.