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

Serguei Katkov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 6 20:58:58 PST 2017


skatkov added a comment.

Hi Daniel,

In https://reviews.llvm.org/D39637#917480, @dberlin wrote:

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


Let me try to explain :)

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

It does not (see code below my patched piece). I do not see an invocation of patchreplacementvalue till line #2305 where replacement actually happens. Moreover as I know phi cannot have nsw/nuw falgs...

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

Yes, I was a bit relaxed in comment saying this. Specifically I meant that we plan to use V value instead of CurInst if control flow will go on the corresponding path.
So, yes you are right, we use V in Phi node which will replace the CurInst.

> Your second test also passes right now without this patch.

Yes, that is true, I just added the second test to emphasize that we do not need patch the original CurInst when we move it to predecessor. It seems to me it is a correct behavior due to it does not introduce new conditions for instruction which uses CurInst.

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

Right, except CurInst which will be moved to predecessor.

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

The problem is as follows. Let's consider the modified version of the first test.
Let return block ends up not by store and ret but with conditional branch, basing on %add.2.
Let add instructions look like:
%add.1 = add nuw nsw i32 %v, -1
%add.2 = add i32 %v, -1

in this case GVN PRE will create a Phi node 
%add.2.pre-phi = phi i32 [ %.pre, %bb1 ], [ %add.1, %bb ]
and condition will be, say %add.2.pre-phi > 0

Now, for example JumpThreading pass basing on
%add.1 = add nuw nsw i32 %v, -1
can prove that %v == 0 (due to nuw) and condition is always false.
so it can eliminate the branch from bb -> return and use the branch bb -> branch corresponding to false in our conditions.

This is incorrect due to original program does not have such semantic.

I hope it helps. If there are other questions, I'll be happy to answer them.


https://reviews.llvm.org/D39637





More information about the llvm-commits mailing list