Page MenuHomePhabricator

[Dominators][CodeGen] Add MachinePostDominatorTree verification
ClosedPublic

Authored by kuhar on Sep 30 2019, 11:27 AM.

Details

Summary

This patch implements Machine PostDominator Tree verification and ensures that the verification doesn't fail the in-tree tests.

MPDT verification can be enabled using verify-machine-dom-info -- the same flag used by Machine Dominator Tree verification.

Flipping the flag revealed that MachineSink falsely claimed to preserve CFG and MDT/MPDT. This patch fixes that.

Diff Detail

Repository
rL LLVM

Event Timeline

kuhar created this revision.Sep 30 2019, 11:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
kuhar updated this revision to Diff 222466.Sep 30 2019, 11:33 AM

Fix a typo

hliao accepted this revision.Sep 30 2019, 12:03 PM

LGTM

This revision is now accepted and ready to land.Sep 30 2019, 12:03 PM
kuhar added inline comments.Sep 30 2019, 12:08 PM
llvm/lib/CodeGen/MachineDominators.cpp
23 ↗(On Diff #222466)

TODO: This should probably be in namespace llvm now.

kuhar updated this revision to Diff 222622.Oct 1 2019, 8:04 AM
kuhar marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.
uabelho added a subscriber: uabelho.Oct 2 2019, 1:33 AM
uabelho added inline comments.
llvm/trunk/lib/CodeGen/MachineSink.cpp
124

Hi,

In my out-of-tree target I'm seeing strange problems with MachineLoopInfo with this patch and I'm wondering if perhaps we should also remove MachineLoopInfo from the preserved passes above?

Since MachineLoopInfo uses MachineDominatorTree, won't MLI also possibly be invalid if MDT is invalid?

kuhar marked an inline comment as done.Oct 2 2019, 6:28 AM
kuhar added inline comments.
llvm/trunk/lib/CodeGen/MachineSink.cpp
124

Hi Mikael,

Over the last couple of days I found a few passes that modify CFG and do not update dominators while claiming to preserve them, which all should be fixed now.

I'm not familiar with MachineLI, but skimming the code it seems that any pass that introduces new blocks and doesn't add it to MLI invalidates it. This seems to be the case for MachineSink, PHIElimination, and maybe some other passes I missed. I think that in those cases the passes have to be fixes to not say they preserve MLI.

Independently, perhaps it would be worth/possible to implement verifyAnalysis for MLI and see where it currently breaks?

uabelho added inline comments.Oct 2 2019, 6:59 AM
llvm/trunk/lib/CodeGen/MachineSink.cpp
124

I'm not familiar with MachineLI, but skimming the code it seems that any pass that introduces new blocks
and doesn't add it to MLI invalidates it. This seems to be the case for MachineSink, PHIElimination, and
maybe some other passes I missed. I think that in those cases the passes have to be fixes to not say they
preserve MLI.

So you also think MachineSink is buggy here and that it should not say it preserves MachineLoopInfo anymore?

Independently, perhaps it would be worth/possible to implement verifyAnalysis for MLI and see where
it currently breaks?

Sounds like a good idea to me.

kuhar marked an inline comment as done.Oct 2 2019, 7:10 AM
kuhar added inline comments.
llvm/trunk/lib/CodeGen/MachineSink.cpp
124

So you also think MachineSink is buggy here and that it should not say it preserves MachineLoopInfo anymore?

Seems so to me. Too know for sure I would have to see a definition of what it means for MLI to be valid.