[ValueTracking] Handle not cond to assume.

[andjo403]

Addresses this comment #126423 (comment).
Also solves the ephemeral issue from #118406 for known bits of x in assume (not trunc x to i1)

Proof: https://alive2.llvm.org/ce/z/Bkg2f6

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Andreas Jonson (andjo403)

Changes

Addresses this comment #126423 (comment).
Also solves the ephemeral issue from #118406 for known bits of x in assume (not trunc x to i1)

is it enough with the test in https://github.com/llvm/llvm-project/blob/3d140004c70e2bc79416825e43207e8b711c56d9/llvm/test/Transforms/GVN/assume.ll ?

---

Full diff: https://github.com/llvm/llvm-project/pull/127140.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+22-16)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 28d7e1ce401e4..1ac37603172a1 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -441,9 +441,6 @@ void llvm::computeKnownBitsFromRangeMetadata(const MDNode &Ranges,
 }
 
 static bool isEphemeralValueOf(const Instruction *I, const Value *E) {
-  SmallVector<const Value *, 16> WorkSet(1, I);
-  SmallPtrSet<const Value *, 32> Visited;
-  SmallPtrSet<const Value *, 16> EphValues;
 
   // The instruction defining an assumption's condition itself is always
   // considered ephemeral to that assumption (even if it has other
@@ -451,6 +448,21 @@ static bool isEphemeralValueOf(const Instruction *I, const Value *E) {
   if (is_contained(I->operands(), E))
     return true;
 
+  SmallPtrSet<const Value *, 32> Visited{I};
+  SmallPtrSet<const Value *, 16> EphValues{I};
+
+  Value *X;
+  if (match(I, m_Intrinsic<Intrinsic::assume>(m_Not(m_Value(X))))) {
+    if (X == E)
+      return true;
+    auto *Not = cast<Instruction>(I->getOperand(0));
+    Visited.insert(Not);
+    EphValues.insert(Not);
+    I = Not;
+  }
+
+  SmallVector<const Value *, 16> WorkSet(I->operands());
+
   while (!WorkSet.empty()) {
     const Value *V = WorkSet.pop_back_val();
     if (!Visited.insert(V).second)
@@ -463,13 +475,11 @@ static bool isEphemeralValueOf(const Instruction *I, const Value *E) {
       if (V == E)
         return true;
 
-      if (V == I || (isa<Instruction>(V) &&
-                     !cast<Instruction>(V)->mayHaveSideEffects() &&
-                     !cast<Instruction>(V)->isTerminator())) {
-       EphValues.insert(V);
-       if (const User *U = dyn_cast<User>(V))
-         append_range(WorkSet, U->operands());
-      }
+      if (const auto *II = dyn_cast<Instruction>(V))
+        if (!II->mayHaveSideEffects() && !II->isTerminator()) {
+          EphValues.insert(V);
+          append_range(WorkSet, II->operands());
+        }
     }
   }
 
@@ -10214,11 +10224,8 @@ void llvm::findValuesAffectedByCondition(
     CmpPredicate Pred;
     Value *A, *B, *X;
 
-    if (IsAssume) {
+    if (IsAssume)
       AddAffected(V);
-      if (match(V, m_Not(m_Value(X))))
-        AddAffected(X);
-    }
 
     if (match(V, m_LogicalOp(m_Value(A), m_Value(B)))) {
       // assume(A && B) is split to -> assume(A); assume(B);
@@ -10302,8 +10309,7 @@ void llvm::findValuesAffectedByCondition(
       // Assume is checked here as X is already added above for assumes in
       // addValueAffectedByCondition
       AddAffected(X);
-    } else if (!IsAssume && match(V, m_Not(m_Value(X)))) {
-      // Assume is checked here to avoid issues with ephemeral values
+    } else if (match(V, m_Not(m_Value(X)))) {
       Worklist.push_back(X);
     }
   }

[andjo403]

Added test and a proof.

[dtcxzyw]

I am ok with the IR diff. But I am a bit confused by this line. Do we really need to specially handle assume(not(cond))?

That is, we should also match trunc here if we want to remove !IsAssume && from the following code:

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Line 10346 in 70b95ca

} else if (!IsAssume && match(V, m_Trunc(m_Value(X)))) {

[andjo403]

the !IsAssume && for the trunc check is only to avoid to add the same value twice to the AssumptionCache as it is added by the

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 10229 to 10232 in 70b95ca

	if (match(I, m_CombineOr(m_PtrToInt(m_Value(Op)), m_Trunc(m_Value(Op))))) {
	if (isa<Instruction>(Op) || isa<Argument>(Op))
	InsertAffected(Op);
	}

via

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 10261 to 10262 in 70b95ca

	if (IsAssume) {
	AddAffected(V);

This was the only way that I was able to think of how to solve supporting not in assumes.

[nikic]

I don't really like special-casing the not here.

What would happen if we treat any instruction that feeds into the assume as ephemeral, even if it has other users?

[andjo403]

What would happen if we treat any instruction that feeds into the assume as ephemeral, even if it has other users?

I maybe missunderstad your question but is that not already done

llvm-project/llvm/lib/Analysis/ValueTracking.cpp

Lines 448 to 452 in 70b95ca

	// The instruction defining an assumption's condition itself is always
	// considered ephemeral to that assumption (even if it has other
	// non-ephemeral users). See r246696's test case for an example.
	if (is_contained(I->operands(), E))
	return true;

the problem is that it only is the argument to the assume function that ignore other users not all instructions that builds up the condition.
added handling for the not as that is the only logical operation that is add to the AssumptionCache as and/or is split up to multiple assumes before added.

[nikic]

I maybe missunderstad your question but is that not already done

I'm suggesting to do that not just for direct assume arguments, but for all instructions. I assume this will have some kind of negative fallout, but I'd like to understand what it is.

[andjo403]

I'm suggesting to do that not just for direct assume arguments, but for all instructions. I assume this will have some kind of negative fallout, but I'd like to understand what it is.

As I understand it isEphemeralValueOf will effectively always return true in that case as it is only called when the value E is before the assume and all instructions that builds up the condition to the assume will be ephemeral.

maybe an other way to make isEphemeralValueOf is to use findValuesAffectedByCondition function to make the ephemeral check like this:

  SmallVector<const Value *, 8> Affected;
  auto InsertAffected = [&Affected](Value *V) { Affected.push_back(V); };
  findValuesAffectedByCondition(Assume->getArgOperand(0), /*IsAssume=*/true,
                                InsertAffected);

  return is_contained(Affected, E) &&
         llvm::all_of(E->users(),
                      [&](const User *U) { return is_contained(Affected, U); });

avoids the need for special handling of not

[andjo403]

But my proposal to use findValuesAffectedByCondition also is no good as it removes the assume in

llvm-project/llvm/test/Transforms/GVN/assume.ll

Lines 48 to 61 in 538b8f8

	define void @pr47496(i8 %x) {
	; CHECK-LABEL: @pr47496(
	; CHECK-NEXT: [[CMP:%.*]] = icmp slt i8 [[X:%.*]], 0
	; CHECK-NEXT: [[XOR:%.*]] = xor i1 [[CMP]], true
	; CHECK-NEXT: call void @llvm.assume(i1 [[XOR]])
	; CHECK-NEXT: call void @use(i1 false)
	; CHECK-NEXT: ret void
	;
	%cmp = icmp slt i8 %x, 0
	%xor = xor i1 %cmp, true
	call void @llvm.assume(i1 %xor)
	call void @use(i1 %cmp)
	ret void
	}

as the %cmp is not ephemeral as there is a user that is not ephemeral

[nikic]

See #128152 for another ephemeral value problem. This is why I think special casing not is not a solution.

I briefly tried the variant where we don't consider extra uses at nikic@37f1d40 / dtcxzyw/llvm-opt-benchmark#2135. That would need more work, but I think that's the general direction we should go to resolve this more thoroughly.