This is an archive of the discontinued LLVM Phabricator instance.

[Dominators][CodeGen] Fix MachineDominatorTree preservation in PHIElimination
ClosedPublic

Authored by kuhar on Sep 27 2019, 12:08 PM.

Details

Summary

PHIElimination modifies CFG and marks MachineDominatorTree as preserved. Therefore, it the CFG changes it should also update the MDT, when available. This patch teaches PHIElimination to recalculate MDT when necessary.

This fixes the tailmerging_in_mbp.ll test failure discovered after switching to generic DomTree verification algorithm in MachineDominators in D67976.

Diff Detail

Repository
rL LLVM

Event Timeline

kuhar created this revision.Sep 27 2019, 12:08 PM
Herald added a project: Restricted Project. · View Herald Transcript
This revision is now accepted and ready to land.Sep 27 2019, 3:36 PM
arsenm added inline comments.Sep 27 2019, 4:15 PM
llvm/lib/CodeGen/PHIElimination.cpp
189–190 ↗(On Diff #222223)

Isn't this basically the same as making it unpreserved? If you aren't going to update at the points it needs, I think it would be better to just stop reporting it as preserved

hiraditya added inline comments.Sep 29 2019, 9:58 AM
llvm/lib/CodeGen/PHIElimination.cpp
189–190 ↗(On Diff #222223)

+1

hiraditya added inline comments.Sep 29 2019, 11:16 AM
llvm/lib/CodeGen/PHIElimination.cpp
189 ↗(On Diff #222223)

Can we put auto *MDT = getAnalysisIfAvailable<MachineDominatorTree>(); when Changed is true?

kuhar planned changes to this revision.Sep 29 2019, 12:34 PM

Thanks for the comments, @arsenm and @hiraditya.

I tried not declaring MDT as a preserved analysis at all, but I got issues with OrcJit unittests:

[2019-09-29 15:31:37.806980254] 0x54de520     Made Modification 'Simple Register Coalescing' on Function 'bar'...
[2019-09-29 15:31:37.807045279] 0x54de520      Freeing Pass 'Simple Register Coalescing' on Function 'bar'...
[2019-09-29 15:31:37.807075980] 0x54de520      Freeing Pass 'MachineDominator Tree Construction' on Function 'bar'...
[2019-09-29 15:31:37.807107079] 0x54de520     Executing Pass 'Rename Disconnected Subregister Components' on Function 'bar'...
[2019-09-29 15:31:37.807177117] 0x54de520      Freeing Pass 'Rename Disconnected Subregister Components' on Function 'bar'...
[2019-09-29 15:31:37.807204793] 0x54de520     Executing Pass 'Machine Instruction Scheduler' on Function 'bar'...
OrcJITTests: /home/kuba/llvm/llvm-project/llvm/include/llvm/PassAnalysisSupport.h:235: AnalysisType &llvm::Pass::getAnalysisID(llvm::AnalysisID) const [AnalysisType = llvm::MachineDominatorTree]: Assertion `ResultPass && "getAnalysis*() called on an analysis that was not " "'required' by pass!"' failed.

Not sure why this is the case, as I marked MDT as a required analysis for MachineScheduler. Do you have some tips here? I'm a newbie when it comes to MIR and Machine passes.

When I preserve the analysis in PHIElimination everything works.

Is there some reason it’s difficult to update the tree where needed here? I would try to just handle that instead of figuring out how to unpreserve it

kuhar added a comment.Sep 29 2019, 1:12 PM

Is there some reason it’s difficult to update the tree where needed here? I would try to just handle that instead of figuring out how to unpreserve it

Will try that, we have a generic updater that we should be able to use.

kuhar added a comment.Sep 30 2019, 7:22 AM

Is there some reason it’s difficult to update the tree where needed here? I would try to just handle that instead of figuring out how to unpreserve it

Will try that, we have a generic updater that we should be able to use.

The issue is that right now MachineDominatorTree has some ad-hoc updater that does (lazy) critical edge splitting before almost any domtree operation. Block splitting uses the old manual API for updating DomTree, and cannot easily be mixed with the new incremental updater.

I think a proper solution here would be to first preserve MDT and MPDT where needed, using the simple recalculation where necessary, then making sure it verifies across machine passes, and later switching to the incremental updater across machine passes instead of the lazy critical edge splitting.

kuhar updated this revision to Diff 222435.Sep 30 2019, 7:56 AM

I'd like to commit this simple fix as-is, make sure no buildbots complain, and then introduce a proper incremental updating in a later revision.

This revision is now accepted and ready to land.Sep 30 2019, 7:56 AM
kuhar marked an inline comment as done.Sep 30 2019, 7:57 AM
kuhar added a comment.Oct 1 2019, 8:25 AM

@arsenm @hiraditya
Are you fine with committing the patch as-is and exploring using the incremental updater later?

This revision was automatically updated to reflect the committed changes.

LGTM, thanks for committing.