Skip to content

[ValueTracking] Bail out on x86_fp80 when computing fpclass with knownbits #130477

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 2 commits into from
Mar 9, 2025

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.

@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
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

LLVM 20 miscompiles/misoptimizes inlined ldexp implementation for x86_fp80
3 participants