Skip to content

Conversation

@dtcxzyw
Copy link
Member

@dtcxzyw dtcxzyw commented Dec 11, 2024

After #118257, we may call computeKnownFPClassFromCond with unrelated conditions. Then miscompilations may occur due to a lack of operand checks.

This bug was introduced by d2404ea and #80740. However, the miscompilation couldn't have happened before #118257, because we only added related conditions to DomConditionCache/AssumptionCache.

Fix the miscompilation reported in #118257 (comment).

@dtcxzyw dtcxzyw requested review from arsenm and asmok-g December 11, 2024 16:19
@dtcxzyw dtcxzyw requested a review from nikic as a code owner December 11, 2024 16:19
@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 Dec 11, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 11, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Yingwei Zheng (dtcxzyw)

Changes

After #118257, we may call computeKnownFPClassFromCond with unrelated conditions. Then miscompilations may occur due to a lack of operand checks.

This bug was introduced by d2404ea and #80740. However, the miscompilation never occurs since we only add related conditions to DomConditionCache/AssumptionCache.

Fix the miscompilation reported in #118257 (comment).


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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+2-2)
  • (modified) llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll (+62)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index f2c6949e535d2a..c148dbce92d1af 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -4906,10 +4906,10 @@ static void computeKnownFPClassFromCond(const Value *V, Value *Cond,
     if (CmpVal == V)
       KnownFromContext.knownNot(~(CondIsTrue ? MaskIfTrue : MaskIfFalse));
   } else if (match(Cond, m_Intrinsic<Intrinsic::is_fpclass>(
-                             m_Value(LHS), m_ConstantInt(ClassVal)))) {
+                             m_Specific(V), m_ConstantInt(ClassVal)))) {
     FPClassTest Mask = static_cast<FPClassTest>(ClassVal);
     KnownFromContext.knownNot(CondIsTrue ? ~Mask : Mask);
-  } else if (match(Cond, m_ICmp(Pred, m_ElementWiseBitCast(m_Value(LHS)),
+  } else if (match(Cond, m_ICmp(Pred, m_ElementWiseBitCast(m_Specific(V)),
                                 m_APInt(RHS)))) {
     bool TrueIfSigned;
     if (!isSignBitCheck(Pred, *RHS, TrueIfSigned))
diff --git a/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll b/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
index 141b44cbbb7a1f..78329faf341727 100644
--- a/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
+++ b/llvm/test/Transforms/InstCombine/fpclass-from-dom-cond.ll
@@ -456,3 +456,65 @@ if.end:
   %ret = call <2 x float> @llvm.fabs.v2f32(<2 x float> %value)
   ret <2 x float> %ret
 }
+
+define i1 @pr118257(half %v0, half %v1) {
+; CHECK-LABEL: define i1 @pr118257(
+; CHECK-SAME: half [[V0:%.*]], half [[V1:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP1:%.*]] = fcmp une half [[V1]], 0xH0000
+; CHECK-NEXT:    [[CAST0:%.*]] = bitcast half [[V0]] to i16
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i16 [[CAST0]], 0
+; CHECK-NEXT:    [[OR_COND:%.*]] = or i1 [[CMP1]], [[CMP2]]
+; CHECK-NEXT:    br i1 [[OR_COND]], label [[IF_END:%.*]], label [[IF_ELSE:%.*]]
+; CHECK:       if.else:
+; CHECK-NEXT:    [[CAST1:%.*]] = bitcast half [[V1]] to i16
+; CHECK-NEXT:    [[CMP3:%.*]] = icmp slt i16 [[CAST1]], 0
+; CHECK-NEXT:    ret i1 [[CMP3]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  %cmp1 = fcmp une half %v1, 0.000000e+00
+  %cast0 = bitcast half %v0 to i16
+  %cmp2 = icmp slt i16 %cast0, 0
+  %or.cond = or i1 %cmp1, %cmp2
+  br i1 %or.cond, label %if.end, label %if.else
+
+if.else:
+  %cast1 = bitcast half %v1 to i16
+  %cmp3 = icmp slt i16 %cast1, 0
+  ret i1 %cmp3
+
+if.end:
+  ret i1 false
+}
+
+define i1 @pr118257_is_fpclass(half %v0, half %v1) {
+; CHECK-LABEL: define i1 @pr118257_is_fpclass(
+; CHECK-SAME: half [[V0:%.*]], half [[V1:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP1:%.*]] = fcmp une half [[V1]], 0xH0000
+; CHECK-NEXT:    [[CMP2:%.*]] = call i1 @llvm.is.fpclass.f16(half [[V0]], i32 35)
+; CHECK-NEXT:    [[OR_COND:%.*]] = or i1 [[CMP1]], [[CMP2]]
+; CHECK-NEXT:    br i1 [[OR_COND]], label [[IF_END:%.*]], label [[IF_ELSE:%.*]]
+; CHECK:       if.else:
+; CHECK-NEXT:    [[CAST1:%.*]] = bitcast half [[V1]] to i16
+; CHECK-NEXT:    [[CMP3:%.*]] = icmp slt i16 [[CAST1]], 0
+; CHECK-NEXT:    ret i1 [[CMP3]]
+; CHECK:       if.end:
+; CHECK-NEXT:    ret i1 false
+;
+entry:
+  %cmp1 = fcmp une half %v1, 0.000000e+00
+  %cmp2 = call i1 @llvm.is.fpclass.half(half %v0, i32 35)
+  %or.cond = or i1 %cmp1, %cmp2
+  br i1 %or.cond, label %if.end, label %if.else
+
+if.else:
+  %cast1 = bitcast half %v1 to i16
+  %cmp3 = icmp slt i16 %cast1, 0
+  ret i1 %cmp3
+
+if.end:
+  ret i1 false
+}

FPClassTest Mask = static_cast<FPClassTest>(ClassVal);
KnownFromContext.knownNot(CondIsTrue ? ~Mask : Mask);
} else if (match(Cond, m_ICmp(Pred, m_ElementWiseBitCast(m_Value(LHS)),
} else if (match(Cond, m_ICmp(Pred, m_ElementWiseBitCast(m_Specific(V)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think this instance is tested

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't it tested by @pr118257?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking the elementwisebitcast would require a vector

@dtcxzyw dtcxzyw merged commit a67bd94 into llvm:main Dec 12, 2024
12 checks passed
@dtcxzyw dtcxzyw deleted the fix-fpclass-from-cmp branch December 12, 2024 02:30
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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants