Skip to content

Conversation

@Maetveis
Copy link
Contributor

@Maetveis Maetveis commented Mar 15, 2025

  • Fix false positive when divisor is a real number.
  • Fix false negative when divident is real, but divisor is complex.
  • Fix false negative when due to promotion the division is performed in higher precision than the divident.
  • Fix false negative in divide and assign (a /= b).

Fixes: #131127

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 15, 2025
@Maetveis Maetveis added clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer labels Mar 15, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2025

@llvm/pr-subscribers-clang

Author: Mészáros Gergely (Maetveis)

Changes
  • Fix false positive when divisor is a real number
  • Fix false negative when divident is real, but divisor is complex
  • Fix false negative when due to promotion the division is performed in higher precision than the divident.
  • Fix false negative in divide and assign (a /= b)

Fixes: #131127


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaExpr.cpp (+38-36)
  • (added) clang/test/Sema/complex-div-warn-higher-precision.c (+58)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index e19136b394800..c13ea9bb40ab3 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -10555,6 +10555,43 @@ static void checkArithmeticNull(Sema &S, ExprResult &LHS, ExprResult &RHS,
       << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
 }
 
+static void DetectPrecisionLossInComplexDivision(Sema &S, QualType DivisorTy,
+                                                 SourceLocation OpLoc) {
+  // Either real/real or complex/real division.
+  // Either way there can be no precision loss.
+  auto *CT = DivisorTy->getAs<ComplexType>();
+  if (!CT)
+    return;
+
+  QualType ElementType = CT->getElementType();
+  bool IsComplexRangePromoted = S.getLangOpts().getComplexRange() ==
+                                LangOptions::ComplexRangeKind::CX_Promoted;
+  if (!ElementType->isFloatingType() || !IsComplexRangePromoted)
+    return;
+
+  ASTContext &Ctx = S.getASTContext();
+  QualType HigherElementType = Ctx.GetHigherPrecisionFPType(ElementType);
+  const llvm::fltSemantics &ElementTypeSemantics =
+      Ctx.getFloatTypeSemantics(ElementType);
+  const llvm::fltSemantics &HigherElementTypeSemantics =
+      Ctx.getFloatTypeSemantics(HigherElementType);
+
+  if (llvm::APFloat::semanticsMaxExponent(ElementTypeSemantics) * 2 + 1 >
+      llvm::APFloat::semanticsMaxExponent(HigherElementTypeSemantics)) {
+    // Retain the location of the first use of higher precision type.
+    if (!S.LocationOfExcessPrecisionNotSatisfied.isValid())
+      S.LocationOfExcessPrecisionNotSatisfied = OpLoc;
+    for (auto &[Type, Num] : S.ExcessPrecisionNotSatisfied) {
+      if (Type == HigherElementType) {
+        Num++;
+        return;
+      }
+    }
+    S.ExcessPrecisionNotSatisfied.push_back(std::make_pair(
+        HigherElementType, S.ExcessPrecisionNotSatisfied.size()));
+  }
+}
+
 static void DiagnoseDivisionSizeofPointerOrArray(Sema &S, Expr *LHS, Expr *RHS,
                                           SourceLocation Loc) {
   const auto *LUE = dyn_cast<UnaryExprOrTypeTraitExpr>(LHS);
@@ -10649,6 +10686,7 @@ QualType Sema::CheckMultiplyDivideOperands(ExprResult &LHS, ExprResult &RHS,
   if (compType.isNull() || !compType->isArithmeticType())
     return InvalidOperands(Loc, LHS, RHS);
   if (IsDiv) {
+    DetectPrecisionLossInComplexDivision(*this, RHS.get()->getType(), Loc);
     DiagnoseBadDivideOrRemainderValues(*this, LHS, RHS, Loc, IsDiv);
     DiagnoseDivisionSizeofPointerOrArray(*this, LHS.get(), RHS.get(), Loc);
   }
@@ -15280,37 +15318,6 @@ static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,
     DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);
 }
 
-static void DetectPrecisionLossInComplexDivision(Sema &S, SourceLocation OpLoc,
-                                                 Expr *Operand) {
-  if (auto *CT = Operand->getType()->getAs<ComplexType>()) {
-    QualType ElementType = CT->getElementType();
-    bool IsComplexRangePromoted = S.getLangOpts().getComplexRange() ==
-                                  LangOptions::ComplexRangeKind::CX_Promoted;
-    if (ElementType->isFloatingType() && IsComplexRangePromoted) {
-      ASTContext &Ctx = S.getASTContext();
-      QualType HigherElementType = Ctx.GetHigherPrecisionFPType(ElementType);
-      const llvm::fltSemantics &ElementTypeSemantics =
-          Ctx.getFloatTypeSemantics(ElementType);
-      const llvm::fltSemantics &HigherElementTypeSemantics =
-          Ctx.getFloatTypeSemantics(HigherElementType);
-      if (llvm::APFloat::semanticsMaxExponent(ElementTypeSemantics) * 2 + 1 >
-          llvm::APFloat::semanticsMaxExponent(HigherElementTypeSemantics)) {
-        // Retain the location of the first use of higher precision type.
-        if (!S.LocationOfExcessPrecisionNotSatisfied.isValid())
-          S.LocationOfExcessPrecisionNotSatisfied = OpLoc;
-        for (auto &[Type, Num] : S.ExcessPrecisionNotSatisfied) {
-          if (Type == HigherElementType) {
-            Num++;
-            return;
-          }
-        }
-        S.ExcessPrecisionNotSatisfied.push_back(std::make_pair(
-            HigherElementType, S.ExcessPrecisionNotSatisfied.size()));
-      }
-    }
-  }
-}
-
 ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
                             tok::TokenKind Kind,
                             Expr *LHSExpr, Expr *RHSExpr) {
@@ -15321,11 +15328,6 @@ ExprResult Sema::ActOnBinOp(Scope *S, SourceLocation TokLoc,
   // Emit warnings for tricky precedence issues, e.g. "bitfield & 0x4 == 0"
   DiagnoseBinOpPrecedence(*this, Opc, TokLoc, LHSExpr, RHSExpr);
 
-  // Emit warnings if the requested higher precision type equal to the current
-  // type precision.
-  if (Kind == tok::TokenKind::slash)
-    DetectPrecisionLossInComplexDivision(*this, TokLoc, LHSExpr);
-
   BuiltinCountedByRefKind K =
       BinaryOperator::isAssignmentOp(Opc) ? AssignmentKind : BinaryExprKind;
 
diff --git a/clang/test/Sema/complex-div-warn-higher-precision.c b/clang/test/Sema/complex-div-warn-higher-precision.c
new file mode 100644
index 0000000000000..73a6e37b82e2c
--- /dev/null
+++ b/clang/test/Sema/complex-div-warn-higher-precision.c
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-linux -verify=no-diag \
+// RUN: -DDIV_CC -DDIV_RC -DDIVASSIGN -DDIVMIXEDFD -DDIVMIXEDID
+
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify=no-diag
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify -DDIV_CC
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify -DDIV_RC
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify -DDIVASSIGN
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify -DDIVMIXEDFD
+// RUN: %clang_cc1 %s -complex-range=promoted -triple x86_64-unknown-windows -verify -DDIVMIXEDID
+
+_Complex double div_ccf(_Complex float a, _Complex float b) {
+    return a / b;
+}
+
+_Complex double div_cr(_Complex double a, double b) {
+    return a / b;
+}
+
+_Complex double div_rr(double a, double b) {
+    return a / b;
+}
+
+_Complex int div_ii(_Complex int a, _Complex int b) {
+    return a / b;
+}
+
+#ifdef DIV_CC
+_Complex double div_cc(_Complex double a, const _Complex double b) {
+    return a / b; // #1
+}
+#endif // DIV_CC
+
+#ifdef DIV_RC
+_Complex double div_rc(double a, _Complex float b) {
+    return a / b; // #1
+}
+#endif // DIV_RC
+
+#ifdef DIVASSIGN
+_Complex double divassign(_Complex double a, _Complex double b) {
+    return a /= b; // #1
+}
+#endif // DIVASSIGN
+
+#ifdef DIVMIXEDFD
+_Complex double divmixedfd(_Complex float a, _Complex double b) {
+    return a / b; // #1
+}
+#endif // DIVMIXEDFD
+
+#ifdef DIVMIXEDID
+_Complex double divmixedid(_Complex int a, _Complex double b) {
+    return a / b; // #1
+}
+#endif // DIVMIXEDID
+
+// no-diag-no-diagnostics
+// expected-warning@#1 {{excess precision is requested but the target does not support excess precision which may result in observable differences in complex division behavior}}

@Maetveis Maetveis added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 15, 2025
@AaronBallman AaronBallman added the floating-point Floating-point math label Mar 19, 2025
@Maetveis Maetveis requested a review from Sirraide March 21, 2025 06:24
@Maetveis Maetveis force-pushed the fix-whigher-precision-for-complex-division branch from a4132f5 to c62bde9 Compare April 7, 2025 09:21
@Maetveis
Copy link
Contributor Author

Maetveis commented Apr 7, 2025

Rebased to fix conflict.
Ping @Sirraide

@Sirraide
Copy link
Member

Sirraide commented Apr 7, 2025

I think the implementation looks fine, but someone more familiar w/ floats than me should approve this.

@Maetveis
Copy link
Contributor Author

Maetveis commented Apr 8, 2025

I think the implementation looks fine, but someone more familiar w/ floats than me should approve this.

Okay, thanks :)
@jcranmer-intel @zahiraam maybe?

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

The changes should come with a release note in clang/docs/ReleaseNotes.rst so users know about the fix.

@AaronBallman AaronBallman requested a review from andykaylor April 8, 2025 12:46
@Maetveis Maetveis force-pushed the fix-whigher-precision-for-complex-division branch 2 times, most recently from a8f5789 to 53aff65 Compare April 9, 2025 08:28
@Maetveis
Copy link
Contributor Author

Maetveis commented Apr 9, 2025

Don't know what's up with buildkite, it looks as if it doesn't like multiple commits? The commit ID its trying to check out is the first in the series, but git fetch -v --prune -- origin refs/pull/131477/head is giving it c62bde990396854bd4015c4ad19894e9297ebe18 which I guess is a squashed commit?

@Maetveis
Copy link
Contributor Author

Maetveis commented Apr 9, 2025

FYI: I found more issues when testing with std::complex: #134980

@zahiraam
Copy link
Contributor

zahiraam commented Apr 9, 2025

Don't know what's up with buildkite, it looks as if it doesn't like multiple commits? The commit ID its trying to check out is the first in the series, but git fetch -v --prune -- origin refs/pull/131477/head is giving it c62bde990396854bd4015c4ad19894e9297ebe18 which I guess is a squashed commit?

Yes, I think the issue here is the multiple commits. You may need to do an interactive rebase.

- Fix false positive when divisor is a real number
- Fix false negative when divident is real, but divisor is complex
- Fix false negative when due to promotion the division is performed
  in higher precision than the divident.
- Fix false negative in divide and assign (`a /= b`)

Fixes: llvm#131127
@Maetveis Maetveis force-pushed the fix-whigher-precision-for-complex-division branch from 53aff65 to d3da72b Compare April 11, 2025 08:29
@Maetveis
Copy link
Contributor Author

I tried a squash + force-push to get buildkite working again.

@Maetveis Maetveis requested a review from zahiraam April 12, 2025 07:42
@Maetveis Maetveis requested a review from zahiraam April 15, 2025 06:23
Copy link
Contributor

@zahiraam zahiraam left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few NIT things to edit. Thanks.

@Maetveis Maetveis merged commit f3c7744 into llvm:main Apr 16, 2025
7 of 11 checks passed
@Maetveis Maetveis deleted the fix-whigher-precision-for-complex-division branch April 16, 2025 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category floating-point Floating-point math

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[clang] Multiple false positives and false negatives in -Whigher-precision-for-complex-division

5 participants