[PATCH] D39637: [GVN PRE] Patch the source for Phi node in PRE

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

TL; DR I'm .. confused.

I don't feel like i understand why your statement is true, and i have trouble seeing why it would be.

It's definitely true that the end result of PRE (IE the resulting phi) should have patchreplacementvalue called when it's used to replace an instruction.
It does, AFAIK.

I don't understand either the comment you've added, or how it relates.
In fact, i literally don't understand the comment.
We aren't using V instead of CurInstr, we are using v in a phi we will later use to replace curinstr.
Those are not the same thing.

Your second test also passes right now without this patch.

It appears you are trying to say that use in a phi node should mean we have to change the nuw/nsw flags on the originals.

It's unclear to me why, and why, if it's not safe, it's okay to PRE things this way.
(IE either you have proven the flags or you have proven the value is different).  It's a different case from replacing one value with another.  When we straight replace one value with another, we are relying on value equality alone.

Here we are relying on path equality as well. Your first example is quite literally saying "along this path, it's this instruction and has nsw nuw flags. Along this other path, it does not have those flags".  That is still true.
I don't see why that would break anything.

Further, right now, -gvn -enable-pre -O2 produces the same result for test1 whether you have the flags are on add.1 or not.

So i don't even see the brokenness you are claiming is happening.

I feel like you need a lot more explanation here.


https://reviews.llvm.org/D39637