[PATCH] D58444: Make MergeBlockIntoPredecessor conformant to the precondition of calling DTU.applyUpdates

[Chijun Sima via Phabricator via llvm-commits]

NutshellySima added a comment.

In D58444#1407194 <https://reviews.llvm.org/D58444#1407194>, @brzycki wrote:

> In D58444#1404380 <https://reviews.llvm.org/D58444#1404380>, @NutshellySima wrote:
>
> > In D58444#1404242 <https://reviews.llvm.org/D58444#1404242>, @brzycki wrote:
> >
> > > Thank you for looking into this. I was working on applying `MergeBLockIntoPredecessor` replacement in `JumpThreading.cpp` (via D48202 <https://reviews.llvm.org/D48202>) and ran into check failures that I haven't had time to properly debug. I suspect at least one of these is related to the change you made above.
> >
> >
> > Thanks for your comment! I am worried about the difficulty in discovering and debugging bugs involving incremental DT updating. I think it is indeed easy to violet the preconditions of `applyUpdates()` and I believe if it is a concern, a strict validation should be added to check if preconditions are fulfilled in DEBUG mode later.
>
>
> @NutshellySima I've spent some time this week working to use the replacement `MergeBLockIntoPredecessor` in `JumpThreading.cpp` along with this patch. I'm still dealing with correctness issues due to slight differences in the API interface assumptions as well as the change of `BB` being hoisted (new) versus `SinglePred` being sunk (old). There are impacts to other analysis like `LVI` and `LoopHeaders` along with changes in the IR due to blocks getting different names.
>
> I'll keep you informed if I pass all correctness tests with my changes.


Thanks! Sounds like it will be possible to avoid DomTree recalculations then. :) I’m glad to help if there is anything involving DTU to the best of my knowledge.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58444