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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

In D109068#2979922 <https://reviews.llvm.org/D109068#2979922>, @anna wrote:

> In D109068#2978422 <https://reviews.llvm.org/D109068#2978422>, @reames wrote:
>
>> 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.
>
> I agree with Philip, so currently, unconditionally calling MSSA verification does some verification, but isn't clear "how much" and we don't have a way to call the more comprehensive one without enabling EXPENSIVE_CHECKS. 
> @asbirlea, I had a slightly different problem :)   I assumed that if we build llvm without EXPENSIVE_CHECKS and commented out the macro where we set VerifyMemorySSA, we should get the *entire* verification for MemorySSA (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/MemorySSA.cpp#L88). If there are additional verifications inside MSSA verifier functionality which is *separately* guarded under EXPENSIVE_CHECKS, that isn't intuitive.
>
> Could  we pls separate out into two different verification levels such as how we do for `DT` (Fast versus Full) and the developer places this the `Full` version under `EXPENSIVE_CHECKS` when we call the verification routines in passes.  This also lets us know exactly what verification we're getting and we can just comment out `EXPENSIVE_CHECKS` for debugging bugs in specific passes. Example: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/LoopSimplifyCFG.cpp#L609

Thank you for the suggestion, I'll send a patch for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109068