[ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition #67282

dc03 · 2023-09-25T04:19:53Z

The second operand of a sdiv/udiv has to be non-null, as division by zero is UB.

Proofs: https://alive2.llvm.org/ce/z/WttZbb

Fixes #64240.

llvmbot · 2023-09-25T04:21:07Z

@llvm/pr-subscribers-llvm-analysis

Changes

The second operand of a udiv has to be non-null, as division by zero is UB.

Proofs: https://alive2.llvm.org/ce/z/PAWdmP

Fixes #64240.

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

2 Files Affected:

(modified) llvm/lib/Analysis/ValueTracking.cpp (+6)
(modified) llvm/test/Analysis/ValueTracking/select-known-non-zero.ll (+24)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 973943790c0aad2..f177321f28cfa2e 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -2263,6 +2263,12 @@ static bool isKnownNonNullFromDominatingCondition(const Value *V,
         return true;
     }
 
+    if (const auto *Div = dyn_cast<BinaryOperator>(U);
+        Div && Div->getOpcode() == BinaryOperator::UDiv &&
+        Div->getOperand(1) == V &&
+        isValidAssumeForContext(cast<Instruction>(U), CtxI, DT))
+      return true;
+
     // Consider only compare instructions uniquely controlling a branch
     Value *RHS;
     CmpInst::Predicate Pred;
diff --git a/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll b/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
index 8b1d2fd0181d66c..a12925ede72ba42 100644
--- a/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
+++ b/llvm/test/Analysis/ValueTracking/select-known-non-zero.ll
@@ -393,3 +393,27 @@ define i1 @inv_select_v_sle_nonneg(i8 %v, i8 %C, i8 %y) {
   %r = icmp eq i8 %s, 0
   ret i1 %r
 }
+
+; Check dominance of the udiv over the icmp.
+define i64 @incorrect_safe_div(i64 %n, i64 %d) {
+; CHECK-LABEL: @incorrect_safe_div(
+; CHECK-NEXT:    [[TMP1:%.*]] = udiv i64 [[N:%.*]], [[D:%.*]]
+; CHECK-NEXT:    ret i64 [[TMP1]]
+;
+  %1 = udiv i64 %n, %d
+  %2 = icmp eq i64 %d, 0
+  %3 = select i1 %2, i64 -1, i64 %1
+  ret i64 %3
+}
+
+; Check post-dominance of the udiv over the icmp.
+define i64 @incorrect_safe_div_post(i64 %n, i64 %d) {
+; CHECK-LABEL: @incorrect_safe_div_post(
+; CHECK-NEXT:    [[TMP1:%.*]] = udiv i64 [[N:%.*]], [[D:%.*]]
+; CHECK-NEXT:    ret i64 [[TMP1]]
+;
+  %1 = icmp eq i64 %d, 0
+  %2 = udiv i64 %n, %d
+  %3 = select i1 %1, i64 -1, i64 %2
+  ret i64 %3
+}

llvm/lib/Analysis/ValueTracking.cpp

dtcxzyw · 2023-09-25T06:02:00Z

llvm/lib/Analysis/ValueTracking.cpp

+    if (const auto *Div = dyn_cast<BinaryOperator>(U);
+        Div && Div->getOpcode() == BinaryOperator::UDiv &&
+        Div->getOperand(1) == V &&
+        isValidAssumeForContext(cast<Instruction>(U), CtxI, DT))


isValidAssumeForContext assumes the first argument is an assume intrinsic. It is incorrect to pass a udiv.

Hmm, I am not sure of any other way to check for post-dominance.

Maybe DT->dominates(cast<Instruction>(U), CtxI)? It guarantees we execute udiv before select. Then we can simplify the pattern in simplifySelectWithICmpCond.

In this case, CtxI is always the icmp, so the dominates call will only work when the udiv dominates the icmp and not the other way round.

isValidAssumeForContext() should be fine to use for non-assumes as well. The key property of assumes here is that violating the fact results in immediate undefined behavior, which is also true for divisions.

Though I guess the isEphemeralValueOf() part of the function is only relevant for assumes...

@nikic Any issues that could occur with isEphemeralValueOf()?

llvm/lib/Analysis/ValueTracking.cpp

nikic · 2023-09-25T08:48:05Z

llvm/lib/Analysis/ValueTracking.cpp

+    if (const auto *Div = dyn_cast<BinaryOperator>(U);
+        Div && Div->getOpcode() == BinaryOperator::UDiv &&
+        Div->getOperand(1) == V &&
+        isValidAssumeForContext(cast<Instruction>(U), CtxI, DT))


isValidAssumeForContext() should be fine to use for non-assumes as well. The key property of assumes here is that violating the fact results in immediate undefined behavior, which is also true for divisions.

Though I guess the isEphemeralValueOf() part of the function is only relevant for assumes...

llvm/test/Analysis/ValueTracking/select-known-non-zero.ll

dtcxzyw · 2023-09-25T12:06:18Z

llvm/lib/Analysis/ValueTracking.cpp

+    if ((match(U, m_UDiv(m_Value(), m_Specific(V))) ||
+         match(U, m_SDiv(m_Value(), m_Specific(V)))) &&


Suggested change

if ((match(U, m_UDiv(m_Value(), m_Specific(V))) ||

match(U, m_SDiv(m_Value(), m_Specific(V)))) &&

if (match(U, m_IDiv(m_Value(), m_Specific(V))) &&

I think it also holds for srem/urem/llvm.sdiv.fix.*/llvm.udiv.fix.*.

@dtcxzyw I think its fine to keep this PR to just IDiv (I'm mainly interested in closing the issue), the others can be implemented separately.

dc03 · 2023-09-25T12:11:58Z

Is this a problem?

diff --git a/llvm/test/Transforms/InstCombine/sdiv-guard.ll b/llvm/test/Transforms/InstCombine/sdiv-guard.ll
index ba9670924108..4894ed90e359 100644
--- a/llvm/test/Transforms/InstCombine/sdiv-guard.ll
+++ b/llvm/test/Transforms/InstCombine/sdiv-guard.ll
@@ -6,9 +6,7 @@ declare void @llvm.experimental.guard(i1, ...)
 ; Regression test. If %flag is false then %s == 0 and guard should be triggered.
 define i32 @a(i1 %flag, i32 %X) nounwind readnone {
 ; CHECK-LABEL: @a(
-; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32 [[X:%.*]], 0
-; CHECK-NEXT:    call void (i1, ...) @llvm.experimental.guard(i1 [[CMP]]) #[[ATTR2:[0-9]+]] [ "deopt"() ]
-; CHECK-NEXT:    [[R:%.*]] = sdiv i32 100, [[X]]
+; CHECK-NEXT:    [[R:%.*]] = sdiv i32 100, [[X:%.*]]
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %s = select i1 %flag, i32 %X, i32 0

nikic · 2023-09-25T12:27:05Z

@dc03 Yeah, that definitely should not be happening...

nikic · 2023-09-28T08:08:43Z

This is a mistake in the test case, I've put up #67641 to fix it.

nikic · 2023-10-19T07:02:47Z

#69433 has been merged, so that should fix the sdiv-guard.ll test case.

…ngCondition The second operand of a udiv has to be non-null, as division by zero is UB. Proofs: https://alive2.llvm.org/ce/z/PAWdmP Fixes llvm#64240.

nikic

LGTM

dc03 requested a review from nikic September 25, 2023 04:19

llvmbot added the llvm:analysis label Sep 25, 2023

dc03 mentioned this pull request Sep 25, 2023

Missed optimization: failure to use div-by-zero UB to delete redundant select #64240

Closed

dtcxzyw reviewed Sep 25, 2023

View reviewed changes

nikic reviewed Sep 25, 2023

View reviewed changes

dtcxzyw changed the title ~~[ValueTracking] Implement udiv support for isKnownNonNullFromDominatingCondition~~ [ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition Sep 25, 2023

dtcxzyw reviewed Sep 25, 2023

View reviewed changes

dc03 added 4 commits October 19, 2023 13:42

[ValueTracking] Implement udiv support for isKnownNonNullFromDominati…

2b8d93f

…ngCondition The second operand of a udiv has to be non-null, as division by zero is UB. Proofs: https://alive2.llvm.org/ce/z/PAWdmP Fixes llvm#64240.

Use pattern matching

0d1a47f

Add sdiv support + Add more tests, clarifying terminology

ee12907

Replace UDiv/SDiv matchers by IDiv

9ffa883

dc03 force-pushed the fix-#61223 branch from ff9b2a4 to 9ffa883 Compare October 19, 2023 08:15

nikic approved these changes Oct 19, 2023

View reviewed changes

dc03 merged commit 076581f into llvm:main Oct 20, 2023
2 checks passed

madhur13490 mentioned this pull request Oct 20, 2023

Revert commit ba8565fbcb975e2d067ce3ae5a7dbaae4953edd3 madhur13490/llvm-project#3

Closed

spupyrev mentioned this pull request Oct 23, 2023

[BOLT] Rename cds to cdsort #69966

Merged

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

lukelmy mentioned this pull request Nov 6, 2024

Use OpBranchConditional instead of OpSelect for ps_udiv in test_shader_instructions HansKristian-Work/vkd3d-proton#2169

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition #67282

[ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition #67282

dc03 commented Sep 25, 2023 •

edited by dtcxzyw

Loading

llvmbot commented Sep 25, 2023

dtcxzyw Sep 25, 2023

dc03 Sep 25, 2023

dtcxzyw Sep 25, 2023

dc03 Sep 25, 2023 •

edited

Loading

nikic Sep 25, 2023

dc03 Oct 19, 2023

nikic Sep 25, 2023

dtcxzyw Sep 25, 2023

dtcxzyw Sep 25, 2023

dc03 Oct 19, 2023

dc03 commented Sep 25, 2023

nikic commented Sep 25, 2023

nikic commented Sep 28, 2023

nikic commented Oct 19, 2023

nikic left a comment

		if ((match(U, m_UDiv(m_Value(), m_Specific(V))) \|\|
		match(U, m_SDiv(m_Value(), m_Specific(V)))) &&

	if ((match(U, m_UDiv(m_Value(), m_Specific(V))) \|\|
	match(U, m_SDiv(m_Value(), m_Specific(V)))) &&
	if (match(U, m_IDiv(m_Value(), m_Specific(V))) &&

[ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition #67282

[ValueTracking] Implement sdiv/udiv support for isKnownNonNullFromDominatingCondition #67282

Conversation

dc03 commented Sep 25, 2023 • edited by dtcxzyw Loading

llvmbot commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dc03 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dc03 commented Sep 25, 2023

nikic commented Sep 25, 2023

nikic commented Sep 28, 2023

nikic commented Oct 19, 2023

nikic left a comment

Choose a reason for hiding this comment

dc03 commented Sep 25, 2023 •

edited by dtcxzyw

Loading

dc03 Sep 25, 2023 •

edited

Loading