Skip to content

[NFC] Remove reflect-error.ll as a failing testcase #124037

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

Closed
wants to merge 1 commit into from

Conversation

inbelic
Copy link
Contributor

@inbelic inbelic commented Jan 23, 2025

  • This testcase was meant to test that an error message was correctly reported from an invoked report_fatal_error
  • However, there doesn't seem to be any precendant of testing this using FileCheck and furthermore it seems unstable

Resolves the buildbots failure from #123853

- This testcase was meant to test that an error message was correctly
reported from an invoked `report_fatal_error`
- However, there doesn't seem to be any precendant of testing this using
FileCheck and furthermore it seems unstable

Resolves the buildbots failures of llvm#123853
@inbelic inbelic marked this pull request as ready for review January 23, 2025 00:36
@llvmbot
Copy link
Member

llvmbot commented Jan 23, 2025

@llvm/pr-subscribers-backend-spir-v

Author: Finn Plummer (inbelic)

Changes
  • This testcase was meant to test that an error message was correctly reported from an invoked report_fatal_error
  • However, there doesn't seem to be any precendant of testing this using FileCheck and furthermore it seems unstable

Resolves the buildbots failure from #123853


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

1 Files Affected:

  • (removed) llvm/test/CodeGen/SPIRV/opencl/reflect-error.ll (-13)
diff --git a/llvm/test/CodeGen/SPIRV/opencl/reflect-error.ll b/llvm/test/CodeGen/SPIRV/opencl/reflect-error.ll
deleted file mode 100644
index 3b3edc13131f58..00000000000000
--- a/llvm/test/CodeGen/SPIRV/opencl/reflect-error.ll
+++ /dev/null
@@ -1,13 +0,0 @@
-; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o /dev/null 2>&1 | FileCheck %s
-; RUN: not llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o /dev/null 2>&1 | FileCheck %s
-
-; CHECK: LLVM ERROR: %{{.*}} = G_INTRINSIC intrinsic(@llvm.spv.reflect), %{{.*}}, %{{.*}} is only supported with the GLSL extended instruction set.
-
-define noundef <4 x float> @reflect_float4(<4 x float> noundef %a, <4 x float> noundef %b) {
-entry:
-  %spv.reflect = call <4 x float> @llvm.spv.reflect.f32(<4 x float> %a, <4 x float> %b)
-  ret <4 x float> %spv.reflect
-}
-
-declare <4 x float> @llvm.spv.reflect.f32(<4 x float>, <4 x float>)
-

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

I don't think we should be deleting a test because it surfaced a bug, and I definitely don't think we should accept that the SPIRV backend not having a practice of testing its error conditions means we shouldn't test them.

We should test error conditions, and we should understand why hwasan is failing on this test because it is likely a bug that we need to address.

@inbelic
Copy link
Contributor Author

inbelic commented Jan 23, 2025

To avoid 'fixing-forward' by integrating this patch, we will wait until #124045 is solved and then we can re-apply.

@inbelic inbelic closed this Jan 23, 2025
@inbelic inbelic deleted the inbelic/remove-reflect-error branch April 2, 2025 18:24
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