[PATCH] D57316: [LoopSimplifyCFG] Use lazy DT update strategy: bug with eager one?

[Max Kazantsev via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Scalar/LoopSimplifyCFG.cpp:325
         Twine(Preheader->getName()).concat("-split"));
     DTU.deleteEdge(Preheader, L.getHeader());
     DTU.insertEdge(NewPreheader, L.getHeader());
----------------
asbirlea wrote:
> I have not looked at this in detail, but I've had a similar issue at one point in SimpleLoopUnswitch with DomTree. AFAICT, this applies for DTU as well.
> Calling deleteEdge and insertEdge one after the other will not do the updates properly.
> The correct way to do it is:
> ```
>   SmallVector<DominatorTree::UpdateType, 3> DTUpdates;
>   DTUpdates.push_back({DT.Insert, NewPreheader, L.getHeader()});
>   DTUpdates.push_back({DT.Insert, Preheader, NewPreheader});
>   DTUpdates.push_back({DT.Delete, Preheader, L.getHeader()});
>   DTU.applyUpdates(DTUpdates);
> ```
> Perhaps the behavior is different with the `Eager` strategy, but I'm fairly confident using `applyUpdates` is correct for DT.
With lazy strategy, `flush()` is doing exactly this thing, calling `applyUpdates` inside.


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

https://reviews.llvm.org/D57316