[ValueTracking] Fix computeKnownFPClass for fpext

[dtcxzyw]

This patch adds the missing subnormal -> normal part for fpext in computeKnownFPClass.
Fixes the miscompilation reported by #80941 (comment).

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch adds the missing subnormal -> normal part for fpext in computeKnownFPClass.
Fixes the miscompilation reported by #80941 (comment).

---

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

3 Files Affected:

• (modified) llvm/lib/Analysis/ValueTracking.cpp (+6-1)
• (modified) llvm/test/Transforms/Attributor/nofpclass-fpext.ll (+32-2)
• (added) llvm/test/Transforms/InstCombine/pr80941.ll (+32)

diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index cc1d5b74dcfc53..1a076adb1bad0a 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -5173,8 +5173,13 @@ void computeKnownFPClass(const Value *V, const APInt &DemandedElts,
         Op->getOperand(0)->getType()->getScalarType()->getFltSemantics();
 
     // All subnormal inputs should be in the normal range in the result type.
-    if (APFloat::isRepresentableAsNormalIn(SrcTy, DstTy))
+    if (APFloat::isRepresentableAsNormalIn(SrcTy, DstTy)) {
+      if (Known.KnownFPClasses & fcPosSubnormal)
+        Known.KnownFPClasses |= fcPosNormal;
+      if (Known.KnownFPClasses & fcNegSubnormal)
+        Known.KnownFPClasses |= fcNegNormal;
       Known.knownNot(fcSubnormal);
+    }
 
     // Sign bit of a nan isn't guaranteed.
     if (!Known.isKnownNeverNaN())
diff --git a/llvm/test/Transforms/Attributor/nofpclass-fpext.ll b/llvm/test/Transforms/Attributor/nofpclass-fpext.ll
index 0ba114117ceec6..ee36f949529d4f 100644
--- a/llvm/test/Transforms/Attributor/nofpclass-fpext.ll
+++ b/llvm/test/Transforms/Attributor/nofpclass-fpext.ll
@@ -142,7 +142,7 @@ define double @ret_fpext_f32_to_f64_nosub(float nofpclass(sub) %arg0) {
 }
 
 define double @ret_fpext_f32_to_f64_nonorm(float nofpclass(norm) %arg0) {
-; CHECK-LABEL: define nofpclass(sub norm) double @ret_fpext_f32_to_f64_nonorm
+; CHECK-LABEL: define nofpclass(sub) double @ret_fpext_f32_to_f64_nonorm
 ; CHECK-SAME: (float nofpclass(sub norm) [[ARG0:%.*]]) #[[ATTR0]] {
 ; CHECK-NEXT:    [[EXT:%.*]] = fpext float [[ARG0]] to double
 ; CHECK-NEXT:    ret double [[EXT]]
@@ -482,7 +482,37 @@ define double @ret_fpext_bf16_f64_nosub(bfloat nofpclass(sub) %arg0) {
 }
 
 define double @ret_fpext_bf16_f64_nonorm(bfloat nofpclass(norm) %arg0) {
-; CHECK-LABEL: define nofpclass(sub norm) double @ret_fpext_bf16_f64_nonorm
+; CHECK-LABEL: define nofpclass(sub) double @ret_fpext_bf16_f64_nonorm
+; CHECK-SAME: (bfloat nofpclass(sub norm) [[ARG0:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT:    [[EXT:%.*]] = fpext bfloat [[ARG0]] to double
+; CHECK-NEXT:    ret double [[EXT]]
+;
+  %ext = fpext bfloat %arg0 to double
+  ret double %ext
+}
+
+define double @ret_fpext_bf16_f64_nonorm_psub(bfloat nofpclass(norm psub) %arg0) {
+; CHECK-LABEL: define nofpclass(sub pnorm) double @ret_fpext_bf16_f64_nonorm_psub
+; CHECK-SAME: (bfloat nofpclass(sub norm) [[ARG0:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT:    [[EXT:%.*]] = fpext bfloat [[ARG0]] to double
+; CHECK-NEXT:    ret double [[EXT]]
+;
+  %ext = fpext bfloat %arg0 to double
+  ret double %ext
+}
+
+define double @ret_fpext_bf16_f64_nonorm_nsub(bfloat nofpclass(norm nsub) %arg0) {
+; CHECK-LABEL: define nofpclass(sub nnorm) double @ret_fpext_bf16_f64_nonorm_nsub
+; CHECK-SAME: (bfloat nofpclass(sub norm) [[ARG0:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT:    [[EXT:%.*]] = fpext bfloat [[ARG0]] to double
+; CHECK-NEXT:    ret double [[EXT]]
+;
+  %ext = fpext bfloat %arg0 to double
+  ret double %ext
+}
+
+define double @ret_fpext_bf16_f64_nonorm_sub(bfloat nofpclass(norm sub) %arg0) {
+; CHECK-LABEL: define nofpclass(sub norm) double @ret_fpext_bf16_f64_nonorm_sub
 ; CHECK-SAME: (bfloat nofpclass(sub norm) [[ARG0:%.*]]) #[[ATTR0]] {
 ; CHECK-NEXT:    [[EXT:%.*]] = fpext bfloat [[ARG0]] to double
 ; CHECK-NEXT:    ret double [[EXT]]
diff --git a/llvm/test/Transforms/InstCombine/pr80941.ll b/llvm/test/Transforms/InstCombine/pr80941.ll
new file mode 100644
index 00000000000000..95242b1d1407bf
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr80941.ll
@@ -0,0 +1,32 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+define float @pr80941(float %arg) {
+; CHECK-LABEL: define float @pr80941(
+; CHECK-SAME: float [[ARG:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[COND:%.*]] = tail call i1 @llvm.is.fpclass.f32(float [[ARG]], i32 144)
+; CHECK-NEXT:    br i1 [[COND]], label [[IF_THEN:%.*]], label [[IF_EXIT:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[FPEXT:%.*]] = fpext float [[ARG]] to double
+; CHECK-NEXT:    [[SIGN:%.*]] = call double @llvm.copysign.f64(double 0.000000e+00, double [[FPEXT]])
+; CHECK-NEXT:    [[FPTRUNC:%.*]] = fptrunc double [[SIGN]] to float
+; CHECK-NEXT:    br label [[IF_EXIT]]
+; CHECK:       if.exit:
+; CHECK-NEXT:    [[RET:%.*]] = phi float [ [[FPTRUNC]], [[IF_THEN]] ], [ [[ARG]], [[ENTRY:%.*]] ]
+; CHECK-NEXT:    ret float [[RET]]
+;
+entry:
+  %cond = tail call i1 @llvm.is.fpclass.f32(float %arg, i32 144)
+  br i1 %cond, label %if.then, label %if.exit
+
+if.then:
+  %fpext = fpext float %arg to double
+  %sign = call double @llvm.copysign.f64(double 0.000000e+00, double %fpext)
+  %fptrunc = fptrunc double %sign to float
+  br label %if.exit
+
+if.exit:
+  %ret = phi float [ %fptrunc, %if.then ], [ %arg, %entry ]
+  ret float %ret
+}

[eaeltsin]

Hi, can we please proceed with this? It is really needed to unblock us!

Thanks!

[goldsteinn]

This LGTM, but don't think I should be only reviewer.

[nikic]

LGTM

[nikic]

It's unfortunate that nofpclass is tested via Attributor, which means it can't be verified with alive.

[dtcxzyw]

It is supported by alive2.
AliveToolkit/alive2@8f12144

Example: https://alive2.llvm.org/ce/z/NuQqsR

[nikic]

Yes, but alive never gets run (https://web.ist.utl.pt/nuno.lopes/alive2/index.php) on these tests because alive does not support inter-procedural passes like Attributor. As such, we don't automatically catch these issues even if they are completely obvious in existing tests.