[Sema] Fix -Wunreachable-code false negative when operands differ only by implicit casts

[codemzs]

Motivation

-Wunreachable-code missed—or in rare cases crashed on—tautological comparisons such as

x != 0 || x != 1.0   // always true
x == 0 && x == 1.0   // always false

when the same variable appears on both sides but one operand goes through a floating‑rank promotion that is target‑dependent. On back‑ends with native half‑precision (_Float16 / __fp16) such as AArch64 +fullfp16, no promotion occurs, so the cast stacks between the two operands differ and the existing heuristic bails out.

Technical description

• Extends Expr::isSameComparisonOperand() – the helper now ignores parentheses and value‑preserving implicit casts (CK_LValueToRValue, floating‑rank CK_FloatingCast) before comparing the underlying operands. This prevents floating‑rank promotions and lvalue‑to‑rvalue conversions from blocking the unreachable‑code diagnostic on targets with native FP16.
  No change needed in CheckIncorrectLogicOperator; it simply benefits from the improved helper.
• Regression test – warn-unreachable_crash.cpp updated to cover both the promoted case (x86‑64) and the native‑half case (AArch64 +fullfp16).
• Docs – release‑note bullet added under Bug Fixes in This Version.

@ziqingluo-90 @yronglin Could you please review promptly? (feel free to also merge it on my behalf) Thanks!

Fixes #149967

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: M. Zeeshan Siddiqui (codemzs)

Changes

Motivation

This is a follow-up fix for bug exposed by the warn-unreachable_crash.cpp test introduced in #142897 , which caused downstream test failures.

Technical description

Teach CheckIncorrectLogicOperator to become cast‑agnostic so that -Wunreachable-code warns correctly when a target provides native scalar half‑precision. The diagnostic previously relied on an implicit FloatingCast that exists on x86‑64 (__fp16 / _Float16 → float) but is absent on AArch64 +fullfp16, RISC‑V +zfh, GPUs, etc.

@ziqingluo-90 @yronglin Could you please review promptly? (feel free to also merge it on my behalf) Thanks!

Fixes #149967

---

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

3 Files Affected:

• (modified) clang/lib/AST/Expr.cpp (+9-2)
• (modified) clang/lib/Analysis/CFG.cpp (+5-2)
• (modified) clang/test/Sema/warn-unreachable_crash.cpp (+29-12)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 2e1a9a3d9ad63..d9aa1ba4f10ba 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -4234,8 +4234,15 @@ bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
       // template parameters.
       const auto *DRE1 = cast<DeclRefExpr>(E1);
       const auto *DRE2 = cast<DeclRefExpr>(E2);
-      return DRE1->isPRValue() && DRE2->isPRValue() &&
-             DRE1->getDecl() == DRE2->getDecl();
+
+      if (DRE1->getDecl() != DRE2->getDecl())
+        return false;
+
+      if ((DRE1->isPRValue() && DRE2->isPRValue()) ||
+          (DRE1->isLValue() && DRE2->isLValue()))
+        return true;
+
+      return false;
     }
     case ImplicitCastExprClass: {
       // Peel off implicit casts.
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index d960d5130332b..3ddb1370a3145 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -1160,8 +1160,11 @@ class CFGBuilder {
       return {};
 
     // Check that it is the same variable on both sides.
-    if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
-      return {};
+    if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2)) {
+      if (!Expr::isSameComparisonOperand(DeclExpr1->IgnoreParenImpCasts(),
+                                         DeclExpr2->IgnoreParenImpCasts()))
+        return {};
+    }
 
     // Make sure the user's intent is clear (e.g. they're comparing against two
     // int literals, or two things from the same enum)
diff --git a/clang/test/Sema/warn-unreachable_crash.cpp b/clang/test/Sema/warn-unreachable_crash.cpp
index 628abcc83f810..dd3995ad4d912 100644
--- a/clang/test/Sema/warn-unreachable_crash.cpp
+++ b/clang/test/Sema/warn-unreachable_crash.cpp
@@ -1,16 +1,33 @@
-// RUN: %clang_cc1 -verify -Wunreachable-code %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -verify -Wunreachable-code %s
+// RUN: %clang_cc1 -triple aarch64-unknown-linux-gnu -target-feature +fullfp16 -verify -Wunreachable-code %s
+// REQUIRES: aarch64-registered-target
 
-// Previously this test will crash
-static void test(__fp16& x) {
-  if (x != 0 || x != 1.0) { // expected-note{{}} no-crash
-      x = 0.9;
-    } else
-      x = 0.8; // expected-warning{{code will never be executed}}
+// =======  __fp16 version  =======
+static void test_fp16(__fp16 &x) {
+  if (x != 0 || x != 1.0) {           // expected-note {{}} no-crash
+    x = 0.9;
+  } else
+    x = 0.8;                          // expected-warning{{code will never be executed}}
 }
 
-static void test2(__fp16& x) {
-  if (x != 1 && x == 1.0) { // expected-note{{}} no-crash
-      x = 0.9; // expected-warning{{code will never be executed}}
-    } else
-      x = 0.8;
+static void test_fp16_b(__fp16 &x) {
+  if (x != 1 && x == 1.0) {           // expected-note {{}} no-crash
+    x = 0.9;                          // expected-warning{{code will never be executed}}
+  } else
+    x = 0.8;
 }
+
+// =======  _Float16 version  =======
+static void test_f16(_Float16 &x) {
+  if (x != 0 || x != 1.0) {           // expected-note {{}} no-crash
+    x = 0.9;
+  } else
+    x = 0.8;                          // expected-warning{{code will never be executed}}
+}
+
+static void test_f16_b(_Float16 &x) {
+  if (x != 1 && x == 1.0) {           // expected-note {{}} no-crash
+    x = 0.9;                          // expected-warning{{code will never be executed}}
+  } else
+    x = 0.8;
+}
\ No newline at end of file

[codemzs]

@gribozavr @sgatev @AaronBallman

Could you please review when you have a moment?

[AaronBallman]

Thank you for working on a fix here! I'm not certain this is the right direction because isSameComparisonOperand expects to get implicit cast nodes and they're now being stripped. So I think some more investigation is needed on the solution.

[codemzs]

Thank you for working on a fix here! I'm not certain this is the right direction because isSameComparisonOperand expects to get implicit cast nodes and they're now being stripped. So I think some more investigation is needed on the solution.

@AaronBallman I have updated the PR with what we discussed. Please let me know if you have any questions.

[AaronBallman]

I think this looks correct to me now, but I'd like to leave the final sign-off to @yronglin because they did the original review on #142897

[yronglin]

LGTM!

[codemzs]

@AaronBallman @yronglin Thank you for the review. I don't have write permissions, can one of you please sign-off and push the change for me? Thanks!

[yronglin]

@AaronBallman @yronglin Thank you for the review. I don't have write permissions, can one of you please sign-off and push the change for me? Thanks!

Sure! Could you add a release notes(in clang/docs/ReleaseNotes.rst Bug Fixes in This Version section)? You can follow the style of previous items.

[codemzs]

@AaronBallman @yronglin Thank you for the review. I don't have write permissions, can one of you please sign-off and push the change for me? Thanks!

Sure! Could you add a release notes(in clang/docs/ReleaseNotes.rst Bug Fixes in This Version section)? You can follow the style of previous items.

@yronglin Done.

[yronglin]

LGTM