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

[Daniel Berlin via llvm-commits]

I think if we just document "this function is not symmetric", that would
help :)


On Mon, May 1, 2017 at 3:32 PM, Davide Italiano <dccitaliano at gmail.com>
wrote:

> 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) ?
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170502/00b292f4/attachment.html>