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

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Dec 5 13:45:16 PST 2017


reames added a comment.

In https://reviews.llvm.org/D39835#922938, @skatkov wrote:

> What is the resolution of the review? Should I land it?


I'm very strongly of the opinion this patch should not land, ever.  I don't believe this change is desirable or necessary.  If I'm reading through the history here correctly, what we're worried about is a two phase transform: 1) we replace a load with a phi of previously stored values, and then 2) we replace that phi with a single add instruction.  It's specifically that *second* transformation which exposes the worried about miscompile.  My argument here is simple: a test case with a *manually written* phi in the same place as our PRE based implementation of FRE inserts one would expose the same miscompile.  Given this, we might very well have a bug in whatever bug does the second transform, but there is nothing wrong with the FRE/PRE here.

In https://reviews.llvm.org/D39835#921468, @dberlin wrote:

> 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.


Everything up to here, I follow and agree with.  Except, PRE just inserts a PHI.  It doesn't "merge the adds".  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?

> 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.

Er, the only correct output here is phi(add, add nsw) or phi(add, add).  phi(add nsw, add nsw) or phi(add nsw, add) would be incorrect.  They're adding the potential poison use (from the nsw flag) along a path not originally present in the source.  Today, right now, PRE produces phi(add, add nsw) which is correct.

> 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.

I can't tell from your example enough detail to know what you're aiming for, but if there's a bug here, it's in replacing the add, not forming the phi node.

> 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.

The phi is entirely correct and could be hand written.  If GVN is considering adds with different flags equivalent without appropriate backpatching, that's a bug in GVN's replacement.  End of story.


https://reviews.llvm.org/D39835





More information about the llvm-commits mailing list