Skip to content

Conversation

@AZero13
Copy link
Contributor

@AZero13 AZero13 commented Jul 28, 2025

Taking inspiration from the way ARM detects overflow, we should check the LHS and RHS as well as the difference when detecting subtraction.

@AZero13 AZero13 marked this pull request as draft July 28, 2025 04:35
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Jul 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 28, 2025

@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-llvm-selectiondag

Author: AZero13 (AZero13)

Changes

This folds better.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp (+19-7)
diff --git a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
index 1764910861df4..cbd42251bc841 100644
--- a/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
@@ -11447,13 +11447,25 @@ void TargetLowering::expandSADDSUBO(
   // For a subtraction, the result should be less than one of the operands
   // (LHS) if and only if the other operand (RHS) is (non-zero) positive,
   // otherwise there will be overflow.
-  SDValue ResultLowerThanLHS = DAG.getSetCC(dl, OType, Result, LHS, ISD::SETLT);
-  SDValue ConditionRHS =
-      DAG.getSetCC(dl, OType, RHS, Zero, IsAdd ? ISD::SETLT : ISD::SETGT);
-
-  Overflow = DAG.getBoolExtOrTrunc(
-      DAG.getNode(ISD::XOR, dl, OType, ConditionRHS, ResultLowerThanLHS), dl,
-      ResultType, ResultType);
+  
+  if (IsAdd) {
+    // For addition, the result should be less than one of the operands (LHS)
+    // if and only if the other operand (RHS) is negative, otherwise there will
+    // be overflow.
+    SDValue ResultLowerThanLHS = DAG.getSetCC(dl, OType, Result, LHS, ISD::SETLT);
+    SDValue ConditionRHS = DAG.getSetCC(dl, OType, RHS, Zero, ISD::SETLT);
+    Overflow = DAG.getBoolExtOrTrunc(
+        DAG.getNode(ISD::XOR, dl, OType, ConditionRHS, ResultLowerThanLHS), dl,
+        ResultType, ResultType);
+  } else {
+    // For subtraction, overflow occurs when the signed comparison of operands
+    // doesn't match the sign of the result
+    SDValue LHSLessThanRHS = DAG.getSetCC(dl, OType, LHS, RHS, ISD::SETLT);
+    SDValue ResultNegative = DAG.getSetCC(dl, OType, Result, Zero, ISD::SETLT);
+    Overflow = DAG.getBoolExtOrTrunc(
+        DAG.getNode(ISD::XOR, dl, OType, LHSLessThanRHS, ResultNegative), dl,
+        ResultType, ResultType);
+  }
 }
 
 bool TargetLowering::expandMULO(SDNode *Node, SDValue &Result,

@github-actions
Copy link

github-actions bot commented Jul 28, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@AZero13
Copy link
Contributor Author

AZero13 commented Jul 28, 2025

Have to rebase to resolve conflicts with ssubo.ll ...

@AZero13 AZero13 force-pushed the cracked branch 3 times, most recently from 7915652 to 1b27e5f Compare July 28, 2025 17:08
@AZero13 AZero13 marked this pull request as ready for review July 28, 2025 17:09
@AZero13 AZero13 requested review from arsenm and jayfoad July 28, 2025 17:09
@topperc
Copy link
Collaborator

topperc commented Jul 28, 2025

The title needs to include GlobalISel and RISCV too since TargetLowering is a SelectionDAG file.

@RKSimon
Copy link
Collaborator

RKSimon commented Jul 28, 2025

Please can you improve the patch summary description.

Taking inspiration from the way ARM detects overflow, we should check the LHS and RHS as well as the difference when detecting subtraction.
@topperc
Copy link
Collaborator

topperc commented Jul 28, 2025

Title should mention SSUBO or sub overflow. Just saying "subtraction" is confusing.

@AZero13 AZero13 changed the title [TargetLowering] Change subtraction to do (LHS < RHS) XOR (RESULT < 0) [TargetLowering] Change SSUBO to do (LHS < RHS) XOR (RESULT < 0) Jul 28, 2025
@AZero13 AZero13 requested a review from topperc July 28, 2025 23:51
@topperc
Copy link
Collaborator

topperc commented Jul 29, 2025

The title needs to include GlobalISel and RISCV too since TargetLowering is a SelectionDAG file.

You changed the title for one of my comments but this is still not addressed

@AZero13 AZero13 changed the title [TargetLowering] Change SSUBO to do (LHS < RHS) XOR (RESULT < 0) [RISCV][GlobalISel][TargetLowering] Change SSUBO to do (LHS < RHS) XOR (RESULT < 0) Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants