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

Fri Feb 22 08:40:57 PST 2019

brzycki added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58444