[PATCH] D39835: [GVN PRE] Clear nsw/nuw for original values in LoadPRE

[Daniel Berlin via llvm-commits]

FWIW: Before you take the absolutist position you have taken, you may want
to consider the below.




> The output of the PRE/FRE here is a phi which looks like this:
> merge:                                            ; preds = %bb1, %bb
>
>   %foo = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]
>
> That's correct.
>



>
> > The only reason it determines the adds are equivalent is because it
> ignores nsw during value numbering. Otherwise, it would not believe they
> were equivalent.
>
> Danny, this is where I completely loose you.  As far as I can tell,
> GVN/PRE is not eliminating the phi.  The output appears to be completely
> correct.  Can you clarify here?
>

GVN is not currently smart enough to eliminate an add using the phi in most
cases, but that's just basically "a feature request away".
Y'all are basically saying "let's not fix this until we have a bug report".
That seems silly to me in this case
It's 100% guaranteed to get the wrong answer at that time without any
further intervention. There is nothing in this code, no testcase, no
nothing, that will cause someone to notice that.
Unless you or I or Eli review it, and remember back to this email thread,
it's going to generate broken code.
You are guaranteed anyone who makes it smart enough to see the phi and the
add are the same (and they are according to the way it value numbers
ignoring flags) will cause a hard to notice bug (this one took years).

Right now it will detect some of the original cases in Scalar PRE and will
get them mostly right (but not for lack of trying).  It will generate IR
that someone will want to obviously improve:
 %foo = load i32, i32* @G, align 4
  %bar.2 = add i32 %v, -1
  %cmp = icmp sgt i32 %foo, 0
  %cmp2 = icmp sgt i32 %bar.2, 0
  store i1 %cmp2, i1* @I, align 4
  br i1 %cmp, label %action, label %return
->
  %bar.2.pre-phi = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]
  %foo = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]
  %cmp = icmp sgt i32 %foo, 0
  %cmp2 = icmp sgt i32 %bar.2.pre-phi, 0
  store i1 %cmp2, i1* @I, align 4
  br i1 %cmp, label %action, label %return

If this goes through any path that isn't scalar PRE, GVN would get this
wrong.
It isn't a PRE problem, and PRE generates crappy and redundant code for it,
so eventually, the likelihood that happens seems to be to be nearly 100%

At the absolute least a comment should be added.
In practice, we should either fix this or add a testcase that will fail
when the day comes that someone improves GVN.

The phi is entirely correct and could be hand written.


Sure, but that's a red herring. You can hand write code with any semantics
you want.

  If GVN is considering adds with different flags equivalent without
> appropriate backpatching, that's a bug in GVN's replacement.  End of story.
>

I am suggesting that rather than add O(N) recursion to backpatching, to
ensure that everything in the def-use chain had it's flags changed, that
we do what we are doing in the scalar PRE case: fixing it when we add the
phi node.
We in fact, don't rely on backpatching to fix it there because it would be
O(N) to do it at backpatching time.

>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171206/f631959b/attachment.html>