[llvm] 9c91614 - [CVP] Guard against poison in common phi value transform (PR50399)

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2021-05-25T20:47:17+02:00
New Revision: 9c91614959f31d743470c1cb2f4828abd8fcb517

URL: https://github.com/llvm/llvm-project/commit/9c91614959f31d743470c1cb2f4828abd8fcb517
DIFF: https://github.com/llvm/llvm-project/commit/9c91614959f31d743470c1cb2f4828abd8fcb517.diff

LOG: [CVP] Guard against poison in common phi value transform (PR50399)

The common phi value transform replaces constants with values that
have the same value as the constant on a given edge. However, LVI
generally only provides information that is correct up to poison,
so this can end up replacing a well-defined value with poison.
D69442 addressed an instance of this problem by clearing poison
flags on the generating instruction, which was sufficient at the
time. rGa917fb89dc28 made LVI's edge value analysis slightly more
powerful, and clearing poison flags is no longer sufficient.

This patch changes the transform to instead explicitly guard against
a poison value instead. This should be satisfied for most cases due
to a prior branch on poison.

Fixes https://bugs.llvm.org/show_bug.cgi?id=50399.

Differential Revision: https://reviews.llvm.org/D102966

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
    llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
index 73ac0933c91eb..36cbd42a5fdd4 100644
--- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
+++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/InstructionSimplify.h"
 #include "llvm/Analysis/LazyValueInfo.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
@@ -193,15 +194,14 @@ static bool simplifyCommonValuePhi(PHINode *P, LazyValueInfo *LVI,
       return false;
   }
 
+  // LVI only guarantees that the value matches a certain constant if the value
+  // is not poison. Make sure we don't replace a well-defined value with poison.
+  // This is usually satisfied due to a prior branch on the value.
+  if (!isGuaranteedNotToBePoison(CommonValue, nullptr, P, DT))
+    return false;
+
   // All constant incoming values map to the same variable along the incoming
-  // edges of the phi. The phi is unnecessary. However, we must drop all
-  // poison-generating flags to ensure that no poison is propagated to the phi
-  // location by performing this substitution.
-  // Warning: If the underlying analysis changes, this may not be enough to
-  //          guarantee that poison is not propagated.
-  // TODO: We may be able to re-infer flags by re-analyzing the instruction.
-  if (auto *CommonInst = dyn_cast<Instruction>(CommonValue))
-    CommonInst->dropPoisonGeneratingFlags();
+  // edges of the phi. The phi is unnecessary.
   P->replaceAllUsesWith(CommonValue);
   P->eraseFromParent();
   ++NumPhiCommon;

diff  --git a/llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll b/llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
index a90d83a0377b9..d51fd55cfe532 100644
--- a/llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
+++ b/llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
@@ -125,17 +125,20 @@ return:
 
 ; The sub has 'nsw', so it is not safe to propagate that value along
 ; the bb2 edge because that would propagate poison to the return.
+; FIXME: In this particular case, it would be possible to perform the
+; transform if we drop nowrap flags from the sub.
 
 define i32 @PR43802(i32 %arg) {
 ; CHECK-LABEL: @PR43802(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    [[SUB:%.*]] = sub i32 0, [[ARG:%.*]]
+; CHECK-NEXT:    [[SUB:%.*]] = sub nsw i32 0, [[ARG:%.*]]
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i32 [[ARG]], -2147483648
 ; CHECK-NEXT:    br i1 [[CMP]], label [[BB2:%.*]], label [[BB3:%.*]]
 ; CHECK:       bb2:
 ; CHECK-NEXT:    br label [[BB3]]
 ; CHECK:       bb3:
-; CHECK-NEXT:    ret i32 [[SUB]]
+; CHECK-NEXT:    [[R:%.*]] = phi i32 [ -2147483648, [[BB2]] ], [ [[SUB]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    ret i32 [[R]]
 ;
 entry:
   %sub = sub nsw i32 0, %arg
@@ -175,11 +178,14 @@ bb3:
   ret i32 %r
 }
 
-; TODO: Miscompile.
+; Similar to the previous case, we know that %y is always poison on the
+; entry -> join1 edge, and thus always zero or poison on the join1 -> join2
+; edge. We need to make sure that we don't replace zero with "zero or poison".
+
 define i8 @pr50399(i8 %x) {
 ; CHECK-LABEL: @pr50399(
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i8 [[X:%.*]], -100
-; CHECK-NEXT:    [[Y:%.*]] = add i8 [[X]], -100
+; CHECK-NEXT:    [[Y:%.*]] = add nsw i8 [[X]], -100
 ; CHECK-NEXT:    br i1 [[CMP]], label [[JOIN1:%.*]], label [[ELSE:%.*]]
 ; CHECK:       else:
 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i8 [[Y]], 0
@@ -189,7 +195,8 @@ define i8 @pr50399(i8 %x) {
 ; CHECK:       else2:
 ; CHECK-NEXT:    br label [[JOIN2]]
 ; CHECK:       join2:
-; CHECK-NEXT:    ret i8 [[Y]]
+; CHECK-NEXT:    [[PHI:%.*]] = phi i8 [ 0, [[JOIN1]] ], [ [[Y]], [[ELSE2]] ]
+; CHECK-NEXT:    ret i8 [[PHI]]
 ;
   %cmp = icmp slt i8 %x, -100
   %y = add nsw i8 %x, -100