[PATCH] D57316: [LoopSimplifyCFG] Work around bug in DTU by using lazy strategy

Wed Jan 30 18:58:20 PST 2019

mkazantsev added a comment.

In D57316#1377173 <https://reviews.llvm.org/D57316#1377173>, @kuhar wrote:

> In D57316#1377047 <https://reviews.llvm.org/D57316#1377047>, @mkazantsev wrote:
>
> > It is indeed a bug in DTU, I was able to construct a unit test to reproduce it. https://bugs.llvm.org/show_bug.cgi?id=40528
>
>
> You should use DTU.applyUpdates after a transformation that affects multiple CFG edges to inform DomTree about all of the things that changed since the last time it was updated. Eager strategy means that at each update point the updates are performed immediately, while the lazy update strategy is free to schedule them at any 'update' (synchronization) point between the first one and flush (possibly multiple time).
>  Perhaps we should improve the the documentation. @mkazantsev: do you have some suggestions?

When I read the documentation, I interpreted

> It is recommended to only use this method when you have exactly one deletion (and no insertions). It is recommended to use applyUpdates() in all other cases.

as a profitability reasoning, not correctness reasoning. If we intentionally do not guarantee equivalence of Eager strategy and Lazy strategy (which is the same as applyUpdates), we should clearly state that "It is only legal to use this method when you have exactly one deletion...".
So if you think that the observed behavior is not a bug, I propose making this change. Otherwise, the bug should be fixed.

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

https://reviews.llvm.org/D57316