Skip to content

Conversation

@dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Mar 9, 2025

In #97762, we assume the minimum possible value of X is NaN implies X is NaN. But it doesn't hold for x86_fp80 format. If the knownbits of X are ?'011111111111110'????????????????????????????????????????????????????????????????, the minimum possible value of X is NaN/unnormal. However, it can be a normal value.

Closes #130408.

@dtcxzyw dtcxzyw requested review from AlexMaclean and arsenm March 9, 2025 09:53
@dtcxzyw dtcxzyw requested a review from nikic as a code owner March 9, 2025 09:53
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Mar 9, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 9, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Yingwei Zheng (dtcxzyw)

Changes

In #97762, we assume the minimum possible value of X is NaN implies X is NaN. But it doesn't hold for x86_fp80 format. If the knownbits of X are ?'011111111111110'????????????????????????????????????????????????????????????????, the minimum possible value of X is NaN/unnormal. However, it can be a normal value.

Closes #130408.


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+2-1)
  • (modified) llvm/test/Transforms/InstSimplify/fcmp.ll (+17)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 08dc34f89130a..2920738445d5b 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6208,13 +6208,14 @@ void computeKnownFPClass(const Value *V, const APInt &DemandedElts,
     else if (Bits.isNegative())
       Known.signBitMustBeOne();
 
-    if (Ty->isIEEE()) {
+    if (Ty->isIEEELikeFPTy()) {
       // IEEE floats are NaN when all bits of the exponent plus at least one of
       // the fraction bits are 1. This means:
       //   - If we assume unknown bits are 0 and the value is NaN, it will
       //     always be NaN
       //   - If we assume unknown bits are 1 and the value is not NaN, it can
       //     never be NaN
+      // Note: They do not hold for x86_fp80 format.
       if (APFloat(Ty->getFltSemantics(), Bits.One).isNaN())
         Known.KnownFPClasses = fcNan;
       else if (!APFloat(Ty->getFltSemantics(), ~Bits.Zero).isNaN())
diff --git a/llvm/test/Transforms/InstSimplify/fcmp.ll b/llvm/test/Transforms/InstSimplify/fcmp.ll
index 64132f5fb7db7..0c2be5210a741 100644
--- a/llvm/test/Transforms/InstSimplify/fcmp.ll
+++ b/llvm/test/Transforms/InstSimplify/fcmp.ll
@@ -16,3 +16,20 @@ define i1 @poison2(float %x) {
   %v = fcmp ueq float %x, poison
   ret i1 %v
 }
+
+define i1 @pr130408(x86_fp80 %x) {
+; CHECK-LABEL: @pr130408(
+; CHECK-NEXT:    [[BITS:%.*]] = bitcast x86_fp80 [[X:%.*]] to i80
+; CHECK-NEXT:    [[MASKED:%.*]] = and i80 [[BITS]], -604444463063240877801473
+; CHECK-NEXT:    [[OR:%.*]] = or i80 [[MASKED]], 302194561415509874573312
+; CHECK-NEXT:    [[FP:%.*]] = bitcast i80 [[OR]] to x86_fp80
+; CHECK-NEXT:    [[RES:%.*]] = fcmp uno x86_fp80 [[FP]], 0xK00000000000000000000
+; CHECK-NEXT:    ret i1 [[RES]]
+;
+  %bits = bitcast x86_fp80 %x to i80
+  %masked = and i80 %bits, -604444463063240877801473
+  %or = or i80 %masked, 302194561415509874573312
+  %fp = bitcast i80 %or to x86_fp80
+  %res = fcmp uno x86_fp80 %fp, 0xK00000000000000000000
+  ret i1 %res
+}

@arsenm
Copy link
Contributor

arsenm commented Mar 9, 2025

Probably should remove the isIEEE from APFloat. It has repeatedly caused confusion and it only points out an irrelevant APFloat implementation detail

@dtcxzyw dtcxzyw merged commit 029e102 into llvm:main Mar 9, 2025
15 checks passed
@dtcxzyw dtcxzyw deleted the fix-130408 branch March 9, 2025 13:10
@dtcxzyw dtcxzyw added this to the LLVM 20.X Release milestone Mar 9, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Mar 9, 2025
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Mar 9, 2025

/cherry-pick 029e102

@llvmbot
Copy link
Member

llvmbot commented Mar 9, 2025

/pull-request #130489

@llvmbot llvmbot moved this from Needs Triage to Done in LLVM Release Status Mar 9, 2025
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Mar 11, 2025
…nbits (llvm#130477)

In llvm#97762, we assume the
minimum possible value of X is NaN implies X is NaN. But it doesn't hold
for x86_fp80 format. If the knownbits of X are
`?'011111111111110'????????????????????????????????????????????????????????????????`,
the minimum possible value of X is NaN/unnormal. However, it can be a
normal value.

Closes llvm#130408.

(cherry picked from commit 029e102)
dtcxzyw added a commit that referenced this pull request Apr 14, 2025
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:analysis Includes value tracking, cost tables and constant folding llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms

Projects

Development

Successfully merging this pull request may close these issues.

LLVM 20 miscompiles/misoptimizes inlined ldexp implementation for x86_fp80

3 participants