[PATCH] D68235: [Dominators][CodeGen] Add MachinePostDominatorTree verification

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar marked an inline comment as done.
kuhar added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/MachineSink.cpp:124
       AU.addRequired<MachineBranchProbabilityInfo>();
-      AU.addPreserved<MachineDominatorTree>();
-      AU.addPreserved<MachinePostDominatorTree>();
       AU.addPreserved<MachineLoopInfo>();
       if (UseBlockFreqInfo)
----------------
uabelho wrote:
> kuhar wrote:
> > uabelho wrote:
> > > 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?
> > > 
> > 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?
> > 
> >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.
> 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.



Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68235/new/

https://reviews.llvm.org/D68235