[PATCH] D32718: [NewGVN] Don't derive incorrect implications.

[Davide Italiano via llvm-commits]

On Mon, May 1, 2017 at 3:26 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>
> On Mon, May 1, 2017 at 2:11 PM, Davide Italiano via Phabricator
> <reviews at reviews.llvm.org> wrote:
>>
>> davide created this revision.
>> Herald added a subscriber: Prazek.
>>
>> The problem is here (debug output):
>>
>>   Processing instruction   %tmp4 = icmp sgt i32 %tmp.0, 0
>>   Created new congruence class for   %tmp4 = icmp sgt i32 %tmp.0, 0 using
>> expression { ExpressionTypeConstant, opcode = 18,  constant = i1 true} at 4
>> and leader i1 true
>>   New class 4 for expression { ExpressionTypeConstant, opcode = 18,
>> constant = i1 true}
>>
>> we believe %tmp1 implies %tmp4
>>
>> where:
>>
>>   br i1 %tmp1, label %bb2, label %bb7
>>   br i1 %tmp4, label %bb5, label %bb7
>>
>> because while looking at PredicateInfo stuffs we end up calling
>> isImpliedTrueByMatchingCmp() with the operands swapped,
>> `y less then x` implies `y less then or equal x` but the other way around
>> is not true.
>
>
> But we also swap the branch predicate when we swap operands.
>
>
>>
>>
>> The function has a very confusing name IMHO. I expect
>> `isImpliedTrueByMatchingCmp` means that *the first argument* is implied by
>> the second and not viceversa. I'm willing to change it, but I wonder if we
>> have other latent bugs in tree because of this.
>
>
> Oh you mean, literally, the isImplied function is being used backwards.
>
> Sigh.
>
> LGTM
>>

Yes, sorry if it wasn't clear enough =)
As this is clearly source of confusion (at least for me), what do you
think of me renaming the function to `impliesTrueByMatchingCmp` (or,
at least document it in the header, or both) ?