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

[Nikita Popov via Phabricator via llvm-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9c91614959f3: [CVP] Guard against poison in common phi value transform (PR50399) (authored by nikic).

Changed prior to commit:
  https://reviews.llvm.org/D102966?vs=347187&id=347746#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102966/new/

https://reviews.llvm.org/D102966

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


Index: llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
===================================================================
--- llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
+++ llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll
@@ -125,17 +125,20 @@
 
 ; 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 @@
   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 @@
 ; 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
Index: llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
+++ 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 @@
       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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D102966.347746.patch
Type: text/x-patch
Size: 3776 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210525/f876f597/attachment.bin>