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

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added a reviewer: spatel.
Herald added a subscriber: hiraditya.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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 <https://reviews.llvm.org/D69442> addressed an instance of this problem by clearing poison flags on the generating instruction, which was sufficient at the time. rGa917fb89dc28 <https://reviews.llvm.org/rGa917fb89dc2818dc329f099e6912e28967961cc9> 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 branch on poison.

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


Repository:
  rG LLVM Github Monorepo

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
@@ -129,13 +129,14 @@
 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
@@ -150,11 +151,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
@@ -164,7 +168,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.347187.patch
Type: text/x-patch
Size: 3513 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210522/36b7bea6/attachment.bin>