[PATCH] D96929: [ValueTracking] Improve impliesPoison

[Juneyoung Lee via Phabricator via llvm-commits]

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

This patch improves ValueTracking's impliesPoison(V1, V2) to do this reasoning:

    %res = call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %a, i64 %b)
    %overflow = extractvalue { i64, i1 } %res, 1
    %mul      = extractvalue { i64, i1 } %res, 0
  
  	; If %mul is poison, %overflow is also poison, and vice versa.

This improvement leads to supporting this optimization under `-instcombine-unsafe-select-transform=0`:

  define i1 @test2_logical(i64 %a, i64 %b, i64* %ptr) {
  ; CHECK-LABEL: @test2_logical(
  ; CHECK-NEXT:    [[MUL:%.*]] = mul i64 [[A:%.*]], [[B:%.*]]
  ; CHECK-NEXT:    [[TMP1:%.*]] = icmp ne i64 [[A]], 0
  ; CHECK-NEXT:    [[TMP2:%.*]] = icmp ne i64 [[B]], 0
  ; CHECK-NEXT:    [[OVERFLOW_1:%.*]] = and i1 [[TMP1]], [[TMP2]]
  ; CHECK-NEXT:    [[NEG:%.*]] = sub i64 0, [[MUL]]
  ; CHECK-NEXT:    store i64 [[NEG]], i64* [[PTR:%.*]], align 8
  ; CHECK-NEXT:    ret i1 [[OVERFLOW_1]]
  ;
  
    %res = tail call { i64, i1 } @llvm.umul.with.overflow.i64(i64 %a, i64 %b)
    %overflow = extractvalue { i64, i1 } %res, 1
    %mul = extractvalue { i64, i1 } %res, 0
    %cmp = icmp ne i64 %mul, 0
    %overflow.1 = select i1 %overflow, i1 true, i1 %cmp
    %neg = sub i64 0, %mul
    store i64 %neg, i64* %ptr, align 8
    ret i1 %overflow.1
  }

Previously, this didn't happen because the flag prevented `select i1 %overflow, i1 true, i1 %cmp` from being `or i1 %overflow, %cmp`.
Note that the select -> or conversion happens only when `impliesPoison(%cmp, %overflow)` returns true.
This improvement allows `impliesPoison` to do the reasoning.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96929

Files:
  llvm/include/llvm/Analysis/ValueTracking.h
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/test/Transforms/InstCombine/umul-sign-check.ll


Index: llvm/test/Transforms/InstCombine/umul-sign-check.ll
===================================================================
--- llvm/test/Transforms/InstCombine/umul-sign-check.ll
+++ llvm/test/Transforms/InstCombine/umul-sign-check.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S %s | FileCheck %s
+; RUN: opt -instcombine -instcombine-unsafe-select-transform=0 -S %s | FileCheck %s
 
 ; Check that we simplify llvm.umul.with.overflow, if the overflow check is
 ; weakened by or (icmp ne %res, 0) %overflow. This is generated by code using
Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -4843,6 +4843,28 @@
   return ::canCreateUndefOrPoison(Op, /*PoisonOnly=*/true);
 }
 
+// Given V of an aggregate type, return true if V's elements are all poison or
+// not poison.
+static bool isAllPoisonOrNot(const Value *V) {
+  assert(V->getType()->isAggregateType());
+
+  auto *II = dyn_cast<IntrinsicInst>(V);
+  if (!II)
+    return false;
+
+  switch(II->getIntrinsicID()) {
+  case Intrinsic::sadd_with_overflow:
+  case Intrinsic::ssub_with_overflow:
+  case Intrinsic::smul_with_overflow:
+  case Intrinsic::uadd_with_overflow:
+  case Intrinsic::usub_with_overflow:
+  case Intrinsic::umul_with_overflow:
+    return true;
+  default:
+    return false;
+  }
+}
+
 static bool directlyImpliesPoison(const Value *ValAssumedPoison,
                                   const Value *V, unsigned Depth) {
   if (ValAssumedPoison == V)
@@ -4853,10 +4875,21 @@
     return false;
 
   const auto *I = dyn_cast<Instruction>(V);
-  if (I && propagatesPoison(cast<Operator>(I))) {
-    return any_of(I->operands(), [=](const Value *Op) {
-      return directlyImpliesPoison(ValAssumedPoison, Op, Depth + 1);
-    });
+  const auto *I2 = dyn_cast<Instruction>(ValAssumedPoison);
+  if (I) {
+    if (propagatesPoison(cast<Operator>(I)))
+      return any_of(I->operands(), [=](const Value *Op) {
+        return directlyImpliesPoison(ValAssumedPoison, Op, Depth + 1);
+      });
+
+    // V  = extractvalue V0, idx
+    // V2 = extractvalue V0, idx2
+    // V0's elements are all poison or not
+    if (isa<ExtractValueInst>(I) && isa<ExtractValueInst>(ValAssumedPoison)) {
+      if (I->getOperand(0) == I2->getOperand(0) &&
+          isAllPoisonOrNot(I->getOperand(0)))
+        return true;
+    }
   }
   return false;
 }
Index: llvm/include/llvm/Analysis/ValueTracking.h
===================================================================
--- llvm/include/llvm/Analysis/ValueTracking.h
+++ llvm/include/llvm/Analysis/ValueTracking.h
@@ -582,6 +582,8 @@
   /// poison.
   /// Formally, given I = `r = op v1 v2 .. vN`, propagatesPoison returns true
   /// if, for all i, r is evaluated to poison or op raises UB if vi = poison.
+  /// If vi is a vector or an aggregate and r is a single value, any poison
+  /// element in vi should make r poison or raise UB.
   /// To filter out operands that raise UB on poison, you can use
   /// getGuaranteedNonPoisonOp.
   bool propagatesPoison(const Operator *I);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D96929.324508.patch
Type: text/x-patch
Size: 3223 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210218/92469265/attachment.bin>