Skip to content

[NFC][SelectionDAG] Replace generic @llvm.expect.with.probability codegen test with X86 test #117848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

antangelo
Copy link
Contributor

Adds test case for X86 to check that the output of @llvm.expect.with.probability's generic lowering is reasonable. This replaces a generic test which only asserts that llc does not crash.

…egen test with X86 test

Adds test case for X86 to check that the output of
@llvm.expect.with.probability's generic lowering is reasonable.
This replaces a generic test which only asserts that llc does not crash.
@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-backend-x86

Author: None (antangelo)

Changes

Adds test case for X86 to check that the output of @llvm.expect.with.probability's generic lowering is reasonable. This replaces a generic test which only asserts that llc does not crash.


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

2 Files Affected:

  • (removed) llvm/test/CodeGen/Generic/builtin-expect-with-probability.ll (-8)
  • (modified) llvm/test/CodeGen/X86/fast-isel-expect.ll (+33-2)
diff --git a/llvm/test/CodeGen/Generic/builtin-expect-with-probability.ll b/llvm/test/CodeGen/Generic/builtin-expect-with-probability.ll
deleted file mode 100644
index aef134b636d5a7..00000000000000
--- a/llvm/test/CodeGen/Generic/builtin-expect-with-probability.ll
+++ /dev/null
@@ -1,8 +0,0 @@
-; RUN: llc < %s
-
-declare i32 @llvm.expect.with.probability(i32, i32, double)
-
-define i32 @test1(i32 %val) nounwind {
-    %expected = call i32 @llvm.expect.with.probability(i32 %val, i32 1, double 0.5)
-    ret i32 %expected
-}
diff --git a/llvm/test/CodeGen/X86/fast-isel-expect.ll b/llvm/test/CodeGen/X86/fast-isel-expect.ll
index 9c2d6488db3c91..e804076304e309 100644
--- a/llvm/test/CodeGen/X86/fast-isel-expect.ll
+++ b/llvm/test/CodeGen/X86/fast-isel-expect.ll
@@ -1,3 +1,4 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
 ; RUN: llc < %s -O0 | FileCheck %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
@@ -6,9 +7,17 @@ target triple = "i686-unknown-linux-gnu"
 @glbl = extern_weak constant i8
 
 declare i64 @llvm.expect.i64(i64, i64)
+declare i64 @llvm.expect.with.probability.i64(i64, i64, double)
 
-define void @test() {
-; CHECK: movl $glbl
+define void @test_expect() {
+; CHECK-LABEL: test_expect:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl $glbl, %eax
+; CHECK-NEXT:    testl %eax, %eax
+; CHECK-NEXT:    jne .LBB0_2
+; CHECK-NEXT:    jmp .LBB0_1
+; CHECK-NEXT:  .LBB0_1: # %bb1
+; CHECK-NEXT:  .LBB0_2: # %bb2
   %cmp2 = icmp eq ptr @glbl, null
   %ext = zext i1 %cmp2 to i64
   %tmp = call i64 @llvm.expect.i64(i64 %ext, i64 0)
@@ -21,3 +30,25 @@ bb1:
 bb2:
   unreachable
 }
+
+define void @test_expect_with_probability() {
+; CHECK-LABEL: test_expect_with_probability:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    movl $glbl, %eax
+; CHECK-NEXT:    testl %eax, %eax
+; CHECK-NEXT:    jne .LBB1_2
+; CHECK-NEXT:    jmp .LBB1_1
+; CHECK-NEXT:  .LBB1_1: # %bb1
+; CHECK-NEXT:  .LBB1_2: # %bb2
+  %cmp2 = icmp eq ptr @glbl, null
+  %ext = zext i1 %cmp2 to i64
+  %tmp = call i64 @llvm.expect.with.probability.i64(i64 %ext, i64 0, double 0.5)
+  %tmp2 = icmp ne i64 %tmp, 0
+  br i1 %tmp2, label %bb1, label %bb2
+
+bb1:
+  unreachable
+
+bb2:
+  unreachable
+}

@RKSimon
Copy link
Collaborator

RKSimon commented Nov 27, 2024

Any reason not to keep the generic test as well to help sniff out regression on other targets?

@antangelo
Copy link
Contributor Author

Any reason not to keep the generic test as well to help sniff out regression on other targets?

I was told to remove it in the GISel patch to fix the same issue: #117835 (comment)

This is my first set of patches to the generic backend code, so I'm not sure what the best testing methodology is or degree to which tests should be generic vs specific to various backends.

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

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

LGTM - I expect other targets will hit issues at some point with this intrinsic, but they can hopefully just copy what x86 does.

@antangelo antangelo merged commit 73f087b into llvm:main Dec 1, 2024
10 checks passed
@antangelo antangelo deleted the expect-with-probability-selectiondag-improve-tests branch December 1, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants