<div dir="ltr">I think if we just document "this function is not symmetric", that would help :)<div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 1, 2017 at 3:32 PM, Davide Italiano <span dir="ltr"><<a href="mailto:dccitaliano@gmail.com" target="_blank">dccitaliano@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, May 1, 2017 at 3:26 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br>
><br>
><br>
> On Mon, May 1, 2017 at 2:11 PM, Davide Italiano via Phabricator<br>
> <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
>><br>
>> davide created this revision.<br>
>> Herald added a subscriber: Prazek.<br>
>><br>
>> The problem is here (debug output):<br>
>><br>
>>   Processing instruction   %tmp4 = icmp sgt i32 %tmp.0, 0<br>
>>   Created new congruence class for   %tmp4 = icmp sgt i32 %tmp.0, 0 using<br>
>> expression { ExpressionTypeConstant, opcode = 18,  constant = i1 true} at 4<br>
>> and leader i1 true<br>
>>   New class 4 for expression { ExpressionTypeConstant, opcode = 18,<br>
>> constant = i1 true}<br>
>><br>
>> we believe %tmp1 implies %tmp4<br>
>><br>
>> where:<br>
>><br>
>>   br i1 %tmp1, label %bb2, label %bb7<br>
>>   br i1 %tmp4, label %bb5, label %bb7<br>
>><br>
>> because while looking at PredicateInfo stuffs we end up calling<br>
>> isImpliedTrueByMatchingCmp() with the operands swapped,<br>
>> `y less then x` implies `y less then or equal x` but the other way around<br>
>> is not true.<br>
><br>
><br>
> But we also swap the branch predicate when we swap operands.<br>
><br>
><br>
>><br>
>><br>
>> The function has a very confusing name IMHO. I expect<br>
>> `isImpliedTrueByMatchingCmp` means that *the first argument* is implied by<br>
>> the second and not viceversa. I'm willing to change it, but I wonder if we<br>
>> have other latent bugs in tree because of this.<br>
><br>
><br>
> Oh you mean, literally, the isImplied function is being used backwards.<br>
><br>
> Sigh.<br>
><br>
> LGTM<br>
>><br>
<br>
</div></div>Yes, sorry if it wasn't clear enough =)<br>
As this is clearly source of confusion (at least for me), what do you<br>
think of me renaming the function to `impliesTrueByMatchingCmp` (or,<br>
at least document it in the header, or both) ?<br>
</blockquote></div><br></div>