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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

Your patch has convinced me that GVN is probably generally broken here, but just happens to not value number or create new phi nodes except for PRE, so it doesn't generate a lot of instances of this issue.
So yeah, i'm fine with the patch in general

I'd appreciate Eli's opinion on the following:

Right now, patchReplacementInstruction has this comment:

  t more restrictive than the value
    // being replaced.
    // Note that if 'I' is a load being replaced by some operation,
    // for example, by an arithmetic operation, then andIRFlags()
    // would just erase all math flags from the original arithmetic
    // operation, which is clearly not wanted and not needed.

I'm unsure this is true.

If we take the example for jump threading,  and make it go through a load (where the load has the value of the two adds), and then replace the load with a phi of the two values, is there a reason we don't have the same problem?



================
Comment at: lib/Transforms/Scalar/GVN.cpp:2292
+    if (Value *V = predMap[i].first) {
+      // We plan to use V instead of CurInst, so patch it.
+      patchReplacementInstruction(CurInst, V);
----------------
Please replace this comment with "If we use an existing value in this phi, we have to patch the original value because the phi will be used to replace a later value"


https://reviews.llvm.org/D39637