Skip to content

[DAG] canCreateUndefOrPoison - add handling for ADD/SUB/MUL overflow nodes #146322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 30, 2025

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented Jun 30, 2025

Neither the arithmetic value or overflow result can create undef/poison from regular operands values.

We have complete test coverage for all ADDO/SUBO nodes, 32-bit codegen handles the _CARRY variants but until #145939 lands AND DAGCombiner::visitFREEZE handles multiple results we can't see any codegen change.

Pulled out of #145939

Neither the arithmetic value or overflow result can create undef/poison from regular operands values.

We have complete test coverage for all ADDO/SUBO nodes, 32-bit codegen handles the _CARRY variants but until llvm#145939 lands AND DAGCombiner::visitFREEZE handles multiple results we can't see any codegen change.

The SMULO/UMULO nodes can be added as well, but I'm struggling to create any test coverage for them.

Pulled out of llvm#145939
@RKSimon RKSimon requested review from arsenm, nikic and bjope June 30, 2025 09:44
@llvmbot llvmbot added backend:X86 llvm:SelectionDAG SelectionDAGISel as well labels Jun 30, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 30, 2025

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Simon Pilgrim (RKSimon)

Changes

Neither the arithmetic value or overflow result can create undef/poison from regular operands values.

We have complete test coverage for all ADDO/SUBO nodes, 32-bit codegen handles the _CARRY variants but until #145939 lands AND DAGCombiner::visitFREEZE handles multiple results we can't see any codegen change.

The SMULO/UMULO nodes can be added as well, but I'm struggling to create any test coverage for them.

Pulled out of #145939


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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+16)
  • (modified) llvm/test/CodeGen/X86/freeze-binary.ll (+6-16)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index fe9a6ea3e77e6..699da5286d13f 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -5557,6 +5557,22 @@ bool SelectionDAG::canCreateUndefOrPoison(SDValue Op, const APInt &DemandedElts,
   case ISD::SPLAT_VECTOR:
     return false;
 
+  case ISD::ADDC:
+  case ISD::SUBC:
+  case ISD::ADDE:
+  case ISD::SUBE:
+  case ISD::SADDO:
+  case ISD::SSUBO:
+  case ISD::SADDO_CARRY:
+  case ISD::SSUBO_CARRY:
+  case ISD::UADDO:
+  case ISD::USUBO:
+  case ISD::UADDO_CARRY:
+  case ISD::USUBO_CARRY:
+    // No poison on result or overflow flags.
+    // TODO: Add SMULO/UMULO with test coverage.
+    return false;
+
   case ISD::SELECT_CC:
   case ISD::SETCC: {
     // Integer setcc cannot create undef or poison.
diff --git a/llvm/test/CodeGen/X86/freeze-binary.ll b/llvm/test/CodeGen/X86/freeze-binary.ll
index 4b25fd8a536b2..189de051011d2 100644
--- a/llvm/test/CodeGen/X86/freeze-binary.ll
+++ b/llvm/test/CodeGen/X86/freeze-binary.ll
@@ -814,11 +814,9 @@ define i32 @freeze_saddo(i32 %a0, i32 %a1, i8 %a2, i8 %a3) nounwind {
 ;
 ; X64-LABEL: freeze_saddo:
 ; X64:       # %bb.0:
-; X64-NEXT:    # kill: def $esi killed $esi def $rsi
-; X64-NEXT:    # kill: def $edi killed $edi def $rdi
+; X64-NEXT:    movl %edi, %eax
 ; X64-NEXT:    addb %cl, %dl
-; X64-NEXT:    adcl $0, %edi
-; X64-NEXT:    leal (%rdi,%rsi), %eax
+; X64-NEXT:    adcl %esi, %eax
 ; X64-NEXT:    retq
   %b = call {i8, i1} @llvm.uadd.with.overflow.i8(i8 %a2, i8 %a3)
   %b.o = extractvalue {i8, i1} %b, 1
@@ -844,11 +842,9 @@ define i32 @freeze_uaddo(i32 %a0, i32 %a1, i8 %a2, i8 %a3) nounwind {
 ;
 ; X64-LABEL: freeze_uaddo:
 ; X64:       # %bb.0:
-; X64-NEXT:    # kill: def $esi killed $esi def $rsi
-; X64-NEXT:    # kill: def $edi killed $edi def $rdi
+; X64-NEXT:    movl %edi, %eax
 ; X64-NEXT:    addb %cl, %dl
-; X64-NEXT:    adcl $0, %edi
-; X64-NEXT:    leal (%rdi,%rsi), %eax
+; X64-NEXT:    adcl %esi, %eax
 ; X64-NEXT:    retq
   %b = call {i8, i1} @llvm.uadd.with.overflow.i8(i8 %a2, i8 %a3)
   %b.o = extractvalue {i8, i1} %b, 1
@@ -878,11 +874,8 @@ define i32 @freeze_ssubo(i32 %a0, i32 %a1, i8 %a2, i8 %a3) nounwind {
 ; X64-LABEL: freeze_ssubo:
 ; X64:       # %bb.0:
 ; X64-NEXT:    movl %edi, %eax
-; X64-NEXT:    xorl %edi, %edi
 ; X64-NEXT:    addb %cl, %dl
-; X64-NEXT:    setb %dil
-; X64-NEXT:    andl $1, %edi
-; X64-NEXT:    subl %edi, %eax
+; X64-NEXT:    sbbl $0, %eax
 ; X64-NEXT:    subl %esi, %eax
 ; X64-NEXT:    retq
   %b = call {i8, i1} @llvm.uadd.with.overflow.i8(i8 %a2, i8 %a3)
@@ -913,11 +906,8 @@ define i32 @freeze_usubo(i32 %a0, i32 %a1, i8 %a2, i8 %a3) nounwind {
 ; X64-LABEL: freeze_usubo:
 ; X64:       # %bb.0:
 ; X64-NEXT:    movl %edi, %eax
-; X64-NEXT:    xorl %edi, %edi
 ; X64-NEXT:    addb %cl, %dl
-; X64-NEXT:    setb %dil
-; X64-NEXT:    andl $1, %edi
-; X64-NEXT:    subl %edi, %eax
+; X64-NEXT:    sbbl $0, %eax
 ; X64-NEXT:    subl %esi, %eax
 ; X64-NEXT:    retq
   %b = call {i8, i1} @llvm.uadd.with.overflow.i8(i8 %a2, i8 %a3)

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't think having explicit test coverage for everything in this list is particularly useful.

@RKSimon RKSimon changed the title [DAG] canCreateUndefOrPoison - add handling for ADD/SUB overflow nodes [DAG] canCreateUndefOrPoison - add handling for ADD/SUB/MUL overflow nodes Jun 30, 2025
@RKSimon RKSimon merged commit b9e4679 into llvm:main Jun 30, 2025
5 of 7 checks passed
@RKSimon RKSimon deleted the dag-nopoison-addsub-overflow branch June 30, 2025 12:27
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…nodes (llvm#146322)

Neither the arithmetic value or overflow result can create undef/poison from regular operands values.

We have complete test coverage for all ADDO/SUBO nodes, 32-bit codegen handles the _CARRY variants but until llvm#145939 lands AND DAGCombiner::visitFREEZE handles multiple results we can't see any codegen change.

Pulled out of llvm#145939
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…nodes (llvm#146322)

Neither the arithmetic value or overflow result can create undef/poison from regular operands values.

We have complete test coverage for all ADDO/SUBO nodes, 32-bit codegen handles the _CARRY variants but until llvm#145939 lands AND DAGCombiner::visitFREEZE handles multiple results we can't see any codegen change.

Pulled out of llvm#145939
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 llvm:SelectionDAG SelectionDAGISel as well
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants