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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

In https://reviews.llvm.org/D39835#921146, @efriedma wrote:

> Sorry, I didn't really look at the jump-threading testcase in the other patch.  (In general, it's bad practice to write testcases which involve multiple passes because it confuses the issue you're actually trying to fix.)  We don't need to strip the nsw in pre-jt-add.ll from the other patch, and we don't need to strip nsw for this testcase.
>
> The PRE transform in the testcase in this patch can be split into two parts: one, we clone the load and move it into the predecessors, and two, we eliminate the fully redundant loads.  Cloning the load is obviously safe: it's the same operation.  Eliminating the fully redundant load is also safe: storing the value to memory and loading it does not change it.  Stores do not erase poison; you just get poisoned memory.


I want to be very clear that we understand what is going on here in case it changes your opinion (but i don't otherwise have one myself, so don't take this as "disagreeing").
We don't actually clone the load. At all.  We don't even consider doing that.  It's not really PRE, it just happens to be a thing that this GVN uses it's PRE infrastructure to notice.

This is a full redundancy elimination by merging existing values without cloning

What's happening is that store to load forwarding in each predecessor determines the value is already equivalent to each add, so replaces the load with a merge of those adds. That's why the stores are there.

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.

In both cases it's quite literally:
pred1:
1 = add nsw
pred2:
2 = add
merge:
3= <some redundant computation>
use 3

->
pred1:
1 = add nsw
pred2:
2 = add
merge:
3 = phi(1, 2)
use 3

What i think i'm hearing you say is that:

if <some redundant computation> was originally "add", and is replaced with phi(add nsw, add) or phi(add nsw, add nsw) that's not okay.
Anything else is okay.

If that is right, then both patches are necessary, but it requires a modified testcase for load pre.

Why:
The scalar PRE patch is obviously necessary in such a world.

So why is the load PRE?

Imagine the above is:
pred1:
1 = add nsw
store
pred2:
2 = add
store
merge:
3= load
4= add
5 = use 4

In such a case, we will first transform this into:
pred1:
1 = add nsw
store
pred2:
2 = add
store
merge:
3= phi(1, 2)
4= add
5 = use 4
Okay so far.

Now we may replace 4 with 3, because they are all just equivalent to the add.
(admittedly, i have not stared at GVN to know whether there are reasons it won't do this, but there is no reason it shouldn't do this :P)

That is just as illegal as the Scalar PRE case by the rules above, as replacing an add with an add nsw.

patchAndReplaceAlluses will not remove the nsw from the phi in this case.
We could teach it to do so.
The other option is this patch, and fixing load PRE when it creates the phi in the first place.


https://reviews.llvm.org/D39835