Skip to content

Conversation

mtrofin
Copy link
Member

@mtrofin mtrofin commented Sep 9, 2025

Modernized it to using update_test_checks​ which addresses an ambgiuty in the previous test formulation, where a profile metadaat of value i32 1​ would have (incorrectly matched.

Copy link
Member Author

mtrofin commented Sep 9, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@mtrofin mtrofin force-pushed the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch from 3dd6205 to b883589 Compare September 9, 2025 04:20
Base automatically changed from users/mtrofin/09-09-_profcheck_require_unknown_metadata_have_a_origin_parameter to main September 10, 2025 22:34
@mtrofin mtrofin force-pushed the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch from b883589 to 0139719 Compare September 10, 2025 22:37
@mtrofin mtrofin marked this pull request as ready for review September 12, 2025 14:59
@llvmbot llvmbot added llvm:instcombine Covers the InstCombine, InstSimplify and AggressiveInstCombine passes llvm:transforms labels Sep 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Mircea Trofin (mtrofin)

Changes

Adding !prof​ annotations to Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll​, and removed llvm-dis​ in favor of using -S

The prof annotations don’t change the intent of the test, but make it resilient to the profcheck​ bot that would have otherwise introduced a branch_weights profile of i32 1, i32 1​, which would have matched the grep.

Added a profile presence test, too.


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

2 Files Affected:

  • (modified) llvm/test/Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll (+8-4)
  • (modified) llvm/utils/profcheck-xfail.txt (-1)
diff --git a/llvm/test/Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll b/llvm/test/Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll
index f558e35ebe015..0b4c852664c2f 100644
--- a/llvm/test/Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll
+++ b/llvm/test/Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll
@@ -1,16 +1,17 @@
-; RUN: opt < %s -passes=instcombine,mem2reg,simplifycfg -simplifycfg-require-and-preserve-domtree=1 | \
-; RUN:   llvm-dis | grep -v store | not grep "i32 1"
+; RUN: opt %s -passes=instcombine,mem2reg,simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S \
+; RUN:   | grep -v store | not grep "i32 1"
+; RUN: opt %s -passes=instcombine,mem2reg,simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
 
 ; Test to make sure that instcombine does not accidentally propagate the load
 ; into the PHI, which would break the program.
 
-define i32 @test(i1 %C) {
+define i32 @test(i1 %C) !prof !0 {
 entry:
         %X = alloca i32         ; <ptr> [#uses=3]
         %X2 = alloca i32                ; <ptr> [#uses=2]
         store i32 1, ptr %X
         store i32 2, ptr %X2
-        br i1 %C, label %cond_true.i, label %cond_continue.i
+        br i1 %C, label %cond_true.i, label %cond_continue.i, !prof !1
 
 cond_true.i:            ; preds = %entry
         br label %cond_continue.i
@@ -22,4 +23,7 @@ cond_continue.i:                ; preds = %cond_true.i, %entry
         ret i32 %tmp.3
 }
 
+; CHECK: %spec.select = select i1 %C, ptr %X, ptr %X2, !prof !1
 
+!0 = !{!"function_entry_count", i64 1000}
+!1 = !{!"branch_weights", i32 2, i32 7}
\ No newline at end of file
diff --git a/llvm/utils/profcheck-xfail.txt b/llvm/utils/profcheck-xfail.txt
index 258e9bcd0b34f..0f2a380a58dcb 100644
--- a/llvm/utils/profcheck-xfail.txt
+++ b/llvm/utils/profcheck-xfail.txt
@@ -831,7 +831,6 @@ Transforms/IndVarSimplify/invalidate-modified-lcssa-phi.ll
 Transforms/IndVarSimplify/pr45835.ll
 Transforms/IndVarSimplify/preserving-debugloc-rem-div.ll
 Transforms/Inline/optimization-remarks-hotness-threshold.ll
-Transforms/InstCombine/2004-09-20-BadLoadCombine2.ll
 Transforms/InstCombine/2004-09-20-BadLoadCombine.ll
 Transforms/InstCombine/2005-04-07-UDivSelectCrash.ll
 Transforms/InstCombine/2011-02-14-InfLoop.ll

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Can you replace this grep test with standard FileCheck + update_test_checks?

@mtrofin mtrofin force-pushed the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch from 0139719 to e3a1591 Compare September 12, 2025 16:42
Copy link
Member Author

mtrofin commented Sep 12, 2025

Can you replace this grep test with standard FileCheck + update_test_checks?

Good point! Done.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, but do you actually still need the prof metadata for your original motivation after this change? I guess the grep was previously matching the wrong thing, which should no longer be a problem. If it's not needed I'd drop it again.

Copy link
Member Author

mtrofin commented Sep 12, 2025

True - it'd get checked by profcheck.

@mtrofin mtrofin force-pushed the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch from e3a1591 to d6811c6 Compare September 12, 2025 16:51
@mtrofin mtrofin force-pushed the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch from d6811c6 to 8cc0a20 Compare September 12, 2025 17:12
@mtrofin mtrofin merged commit 0d4a615 into main Sep 12, 2025
5 of 8 checks passed
@mtrofin mtrofin deleted the users/mtrofin/09-09-_instcombine_make_test_resilient_to_metadata_presence branch September 12, 2025 22:07
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

None yet

Development

Successfully merging this pull request may close these issues.

3 participants