[PATCH] D56284: [UnrollRuntime] Fix domTree failure in multiexit unrolling

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In D56284#1348783 <https://reviews.llvm.org/D56284#1348783>, @kuhar wrote:

> In D56284#1348747 <https://reviews.llvm.org/D56284#1348747>, @anna wrote:
>
> > I've got a naive question about the DTU: Does the automatic updater handle the ImmediateDominator "automatically" for the remaining nodes in the dom tree once identify that one of the nodes in the DT has a change in IDom?
>
>
> Exactly. Given a set of CFG updates, if figures out how to change the DomTree and PostDomTree such that they match the CFG.
>
> > This will drastically help with #2 because what I have as a local fix using the old DT is something like this:
> > 
> >   +   if (DT) {
> >   +    // Check the dom children of each block in loop and if it is outside the
> >   +    // current loop, update it to the preheader.
> >   +    for (auto *BB: L->blocks()) {
> >   +      auto *DomNodeBB = DT->getNode(BB);
> >   +      for (auto *DomChild: DomNodeBB->getChildren()) {
> >   +        if (!L->contains(LI->getLoopFor(DomChild->getBlock()))) {
> >   +          DT->changeImmediateDominator(DomChild, DT->getNode(PreHeader));
> >   +        }    
> >   +      }    
> >   +    }
> >   +   }
> > 
> > 
> > This is a more general fix using the old DT to handle the non-immediate successors of an exit block in the loop.
>
> Doesn't this loop have to be run until you reach a fixpoint?


yes, that's right - I'd missed that. Now, It's a principled fix which handles all cases of nearest common dominator of the IDoms of multiple exiting blocks,  and these other cases I've seen above.

If any more DT bugs shows up in our fuzzer run, I'm just going to scrap this and go with the DTU itself instead of the other way around - landing this and then changing to DTU :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56284