[PATCH] D109068: Fix a missing memoryssa update in breakLoopBackedge

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D109068#2978419 <https://reviews.llvm.org/D109068#2978419>, @asbirlea wrote:

> In D109068#2978402 <https://reviews.llvm.org/D109068#2978402>, @reames wrote:
>
>> In D109068#2977909 <https://reviews.llvm.org/D109068#2977909>, @asbirlea wrote:
>>
>>> The reason the verification didn't trigger right after LoopDeletion is that it's under EXPENSIVE_CHECKS (in verifyOrderingDominationAndDefUses). MSSA is incorrect (without this change) after LoopDeletion.
>>
>> FYI, this is not the sole issue.  I manually added a call to MSSA->verifyMemorySSA() unconditionally to the end of the pass, and it did not catch this.
>
> I'm sorry, I didn't fully parse this.
> Adding MSSA->verifyMemorySSA() unconditionally to the end of the pass will not catch this. You need to enable EXPENSIVE_CHECKS (or uncomment the ifdefs inside verifyOrderingDominationAndDefUses for this case, there's more in another verify method). Part of the verification is disabled, even in debug build.
> Or "by not sole issue" did you mean there's an additional issue not fixed by this patch?

I'd meant that a missing call to verify memory SSA was not the sole issue.  Your explanation explains why an expensive checks build found this, but local testing (on an asserts build) did not.

However, please change this.  Calling verifyMemorySSA should not change behavior based on EXPENSIVE_CHECKS!! EXPENSIVE_CHECKS can decide whether to call verify, or even change a "how hard" flag to the verify routine, but a manual call to verify, should verify.  It's an idiomatic pattern for all of our analyzes to be able to scatter verify calls under debug options, and have that locally catch problems in assert builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109068