Skip to content

Conversation

dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Apr 23, 2025

This patch disables the fold for logical is_finite test (i.e., and (fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf).
It is still possible to allow this fold for several logical cases (e.g., stripSignOnlyFPOps(RHS0) does not strip any operations). Since this patch has no real-world impact, I decided to disable this fold for all logical cases.

Alive2: https://alive2.llvm.org/ce/z/aH4LC7
Closes #136650.

@dtcxzyw
Copy link
Member Author

dtcxzyw commented Apr 23, 2025

No real-world impact.

@dtcxzyw dtcxzyw requested a review from arsenm April 23, 2025 12:21
@dtcxzyw dtcxzyw marked this pull request as ready for review April 23, 2025 12:22
@dtcxzyw dtcxzyw requested a review from nikic as a code owner April 23, 2025 12:22
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Apr 23, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

This patch disables the fold for logical is_finite test (i.e., and (fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf).
It is still possible to allow this fold for several logical cases (e.g., stripSignOnlyFPOps(RHS0) does not strip any operations). Since this patch has no real-world impact, I decided to disable this fold for all logical cases.

Alive2: https://alive2.llvm.org/ce/z/aH4LC7
Closes #136650.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+3-1)
  • (modified) llvm/test/Transforms/InstCombine/and-fcmp.ll (+28)
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index f1b225c0f238a..979a9cbedf2ef 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -1480,7 +1480,9 @@ Value *InstCombinerImpl::foldLogicOfFCmps(FCmpInst *LHS, FCmpInst *RHS,
     }
   }
 
-  if (IsAnd && stripSignOnlyFPOps(LHS0) == stripSignOnlyFPOps(RHS0)) {
+  // This transform is not valid for a logical select.
+  if (!IsLogicalSelect && IsAnd &&
+      stripSignOnlyFPOps(LHS0) == stripSignOnlyFPOps(RHS0)) {
     // and (fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf
     // and (fcmp ord x, 0), (fcmp u* fabs(x), inf) -> fcmp o* x, inf
     if (Value *Left = matchIsFiniteTest(Builder, LHS, RHS))
diff --git a/llvm/test/Transforms/InstCombine/and-fcmp.ll b/llvm/test/Transforms/InstCombine/and-fcmp.ll
index c7bbc8ab56f9a..ec1b6ad2ea168 100644
--- a/llvm/test/Transforms/InstCombine/and-fcmp.ll
+++ b/llvm/test/Transforms/InstCombine/and-fcmp.ll
@@ -4990,6 +4990,34 @@ define i1 @clang_builtin_isnormal_inf_check_copysign(half %x, half %y) {
   ret i1 %and
 }
 
+define i1 @clang_builtin_isnormal_inf_check_copysign_logical_select(half %x, half %y) {
+; CHECK-LABEL: @clang_builtin_isnormal_inf_check_copysign_logical_select(
+; CHECK-NEXT:    [[COPYSIGN_X:%.*]] = call half @llvm.copysign.f16(half [[X:%.*]], half [[Y:%.*]])
+; CHECK-NEXT:    [[ORD:%.*]] = fcmp ord half [[X]], 0xH0000
+; CHECK-NEXT:    [[CMP:%.*]] = fcmp ueq half [[COPYSIGN_X]], 0xH7C00
+; CHECK-NEXT:    [[AND:%.*]] = select i1 [[ORD]], i1 [[CMP]], i1 false
+; CHECK-NEXT:    ret i1 [[AND]]
+;
+  %copysign.x = call half @llvm.copysign.f16(half %x, half %y)
+  %ord = fcmp ord half %x, 0.0
+  %cmp = fcmp uge half %copysign.x, 0xH7C00
+  %and = select i1 %ord, i1 %cmp, i1 false
+  ret i1 %and
+}
+
+define i1 @clang_builtin_isnormal_inf_check_fabs_nnan_logical_select(half %x) {
+; CHECK-LABEL: @clang_builtin_isnormal_inf_check_fabs_nnan_logical_select(
+; CHECK-NEXT:    [[COPYSIGN_X:%.*]] = call half @llvm.fabs.f16(half [[X:%.*]])
+; CHECK-NEXT:    [[AND:%.*]] = fcmp oeq half [[COPYSIGN_X]], 0xH7C00
+; CHECK-NEXT:    ret i1 [[AND]]
+;
+  %copysign.x = call nnan half @llvm.fabs.f16(half %x)
+  %ord = fcmp ord half %x, 0.0
+  %cmp = fcmp uge half %copysign.x, 0xH7C00
+  %and = select i1 %ord, i1 %cmp, i1 false
+  ret i1 %and
+}
+
 define i1 @isnormal_logical_select_0(half %x) {
 ; CHECK-LABEL: @isnormal_logical_select_0(
 ; CHECK-NEXT:    [[FABS_X:%.*]] = call half @llvm.fabs.f16(half [[X:%.*]])

@dtcxzyw dtcxzyw merged commit 8abc917 into llvm:main Apr 23, 2025
16 checks passed
@dtcxzyw dtcxzyw deleted the fix-136650 branch April 23, 2025 16:12
@dtcxzyw dtcxzyw added this to the LLVM 20.X Release milestone Apr 28, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Apr 28, 2025
@dtcxzyw
Copy link
Member Author

dtcxzyw commented Apr 28, 2025

/cherry-pick 8abc917

@llvmbot
Copy link
Member

llvmbot commented Apr 28, 2025

/pull-request #137606

@llvmbot llvmbot moved this from Needs Triage to Done in LLVM Release Status Apr 28, 2025
swift-ci pushed a commit to swiftlang/llvm-project that referenced this pull request Apr 29, 2025
This patch disables the fold for logical is_finite test (i.e., `and
(fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf`).
It is still possible to allow this fold for several logical cases (e.g.,
`stripSignOnlyFPOps(RHS0)` does not strip any operations). Since this
patch has no real-world impact, I decided to disable this fold for all
logical cases.

Alive2: https://alive2.llvm.org/ce/z/aH4LC7
Closes llvm#136650.

(cherry picked from commit 8abc917)
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
This patch disables the fold for logical is_finite test (i.e., `and
(fcmp ord x, 0), (fcmp u* x, inf) -> fcmp o* x, inf`).
It is still possible to allow this fold for several logical cases (e.g.,
`stripSignOnlyFPOps(RHS0)` does not strip any operations). Since this
patch has no real-world impact, I decided to disable this fold for all
logical cases.

Alive2: https://alive2.llvm.org/ce/z/aH4LC7
Closes llvm#136650.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms
Projects
Development

Successfully merging this pull request may close these issues.

[InstCombine] Poison safety violation in foldLogicOfFCmps
4 participants