[PATCH] D23184: Rewrite domination verifier to handle local domination as well.

[George Burgess IV via llvm-commits]

SGTM :)

On Fri, Aug 5, 2016 at 12:24 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>
> On Fri, Aug 5, 2016 at 12:04 PM, George Burgess IV <
> george.burgess.iv at gmail.com> wrote:
>
>> george.burgess.iv accepted this revision.
>> george.burgess.iv added a comment.
>> This revision is now accepted and ready to land.
>>
>> LGTM with comments, thanks.
>>
>>
>> ================
>> Comment at: include/llvm/Transforms/Utils/MemorySSA.h:594
>> @@ +593,3 @@
>> +  /// dominates Use \p B.
>> +  bool dominates(const MemoryAccess *A, const Use &B) const;
>> +
>> ----------------
>> Do we see this being used outside of verification?
>
>
> Yes, actually.
> It also matches what Dominators.h provides, and that gets used.
>
>
>> If not, make this private, please.
>>
>> ================
>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1872
>> @@ -1891,1 +1871,3 @@
>> +      for (const Use &U : MP->uses())
>> +        assert(dominates(MP, U) && "Memory PHI does not dominate it's
>> uses");
>>      for (Instruction &I : B) {
>> ----------------
>> Looks like this definition is provided if `NDEBUG` is defined. Please
>> braces around the assert. :)
>>
>> ================
>> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1879
>> @@ -1913,1 +1878,3 @@
>> +      for (const Use &U : MD->uses())
>> +        assert(dominates(MD, U) && "Memory Def does not dominate it's
>> uses");
>>      }
>> ----------------
>> Same as above
>>
>>
>> https://reviews.llvm.org/D23184
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160805/08dda29a/attachment.html>