Skip to content

Conversation

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Oct 6, 2025

Add header-side undef hooks for:
__opencl_c_integer_dot_product_input_4x8bit
__opencl_c_integer_dot_product_input_4x8bit_packed

Motivation: our downstream legacy targets lack cl_khr_integer_dot_product builtins (similar to a60b8f4). Providing undef* macros lets users suppress the feature macros without patching headers.

Fix intel/compute-runtime#817

Add header-side undef hooks for:
  __opencl_c_integer_dot_product_input_4x8bit
  __opencl_c_integer_dot_product_input_4x8bit_packed

Motivation: our downstream legacy targets lack cl_khr_integer_dot_product
builtins (similar to a60b8f4). Providing __undef__* macros lets
users suppress the feature macros without patching headers.

Fix intel/compute-runtime#817
@wenju-he wenju-he requested a review from Copilot October 6, 2025 09:42
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Oct 6, 2025
@wenju-he wenju-he requested a review from svenvh October 6, 2025 09:42
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds header-side undefined hooks for the OpenCL integer dot product feature macros to allow downstream legacy targets to suppress these features without patching headers. The changes enable users to define __undef__* macros to disable the cl_khr_integer_dot_product features when they are not supported by their targets.

Key changes:

  • Added undef hooks for __opencl_c_integer_dot_product_input_4x8bit and __opencl_c_integer_dot_product_input_4x8bit_packed feature macros
  • Added corresponding test coverage to verify the undef functionality works correctly

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
clang/lib/Headers/opencl-c-base.h Added conditional undef blocks for the two integer dot product feature macros
clang/test/SemaOpenCL/features.cl Added test case to verify the undef macros properly suppress the feature definitions

@llvmbot
Copy link
Member

llvmbot commented Oct 6, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Wenju He (wenju-he)

Changes

Add header-side undef hooks for:
__opencl_c_integer_dot_product_input_4x8bit
__opencl_c_integer_dot_product_input_4x8bit_packed

Motivation: our downstream legacy targets lack cl_khr_integer_dot_product builtins (similar to a60b8f4). Providing undef* macros lets users suppress the feature macros without patching headers.

Fix intel/compute-runtime#817


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

2 Files Affected:

  • (modified) clang/lib/Headers/opencl-c-base.h (+7)
  • (modified) clang/test/SemaOpenCL/features.cl (+9)
diff --git a/clang/lib/Headers/opencl-c-base.h b/clang/lib/Headers/opencl-c-base.h
index 6206a347852be..7de10c6ecd6a3 100644
--- a/clang/lib/Headers/opencl-c-base.h
+++ b/clang/lib/Headers/opencl-c-base.h
@@ -53,6 +53,13 @@
 #define __opencl_c_kernel_clock_scope_work_group 1
 #define __opencl_c_kernel_clock_scope_sub_group 1
 
+#ifdef __undef___opencl_c_integer_dot_product_input_4x8bit
+#undef __opencl_c_integer_dot_product_input_4x8bit
+#endif
+#ifdef __undef___opencl_c_integer_dot_product_input_4x8bit_packed
+#undef __opencl_c_integer_dot_product_input_4x8bit_packed
+#endif
+
 #endif // defined(__SPIR__) || defined(__SPIRV__)
 #endif // (defined(__OPENCL_CPP_VERSION__) || __OPENCL_C_VERSION__ >= 200)
 
diff --git a/clang/test/SemaOpenCL/features.cl b/clang/test/SemaOpenCL/features.cl
index 3f59b4ea3b5ae..dd82689c415ad 100644
--- a/clang/test/SemaOpenCL/features.cl
+++ b/clang/test/SemaOpenCL/features.cl
@@ -26,6 +26,12 @@
 // RUN: %clang_cc1 -triple spir-unknown-unknown %s -E -dM -o - -x cl -cl-std=clc++1.0 \
 // RUN:   | FileCheck -match-full-lines %s  --check-prefix=NO-FEATURES
 
+// For OpenCL C 2.0, header-only features can be disabled using macros.
+// RUN: %clang_cc1 -triple spir-unknown-unknown %s -E -dM -o - -x cl -cl-std=CL2.0 -fdeclare-opencl-builtins -finclude-default-header \
+// RUN:    -D__undef___opencl_c_integer_dot_product_input_4x8bit \
+// RUN:    -D__undef___opencl_c_integer_dot_product_input_4x8bit_packed \
+// RUN:   | FileCheck %s --check-prefix=NO-HEADERONLY-FEATURES-CL20
+
 // For OpenCL C 3.0, header-only features can be disabled using macros.
 // RUN: %clang_cc1 -triple spir-unknown-unknown %s -E -dM -o - -x cl -cl-std=CL3.0 -fdeclare-opencl-builtins -finclude-default-header \
 // RUN:    -D__undef___opencl_c_work_group_collective_functions=1 \
@@ -64,6 +70,9 @@
 // NO-FEATURES-NOT: #define __opencl_c_read_write_images
 // NO-FEATURES-NOT: #define __opencl_c_subgroups
 
+// NO-HEADERONLY-FEATURES-CL20-NOT: #define __opencl_c_integer_dot_product_input_4x8bit
+// NO-HEADERONLY-FEATURES-CL20-NOT: #define __opencl_c_integer_dot_product_input_4x8bit_packed
+
 // NO-HEADERONLY-FEATURES-NOT: #define __opencl_c_work_group_collective_functions
 // NO-HEADERONLY-FEATURES-NOT: #define __opencl_c_atomic_order_seq_cst
 // NO-HEADERONLY-FEATURES-NOT: #define __opencl_c_atomic_scope_device

@wenju-he wenju-he requested a review from bader October 6, 2025 09:45
@wenju-he
Copy link
Contributor Author

wenju-he commented Oct 6, 2025

For simplicity, can we revert https://reviews.llvm.org/D92231 and don't always define the feature macro in headers?
It would be easier for users. For instance, as @aratajew points out, Mesa workarounds the issue by undefining __SPIR__ and __SPIRV__, see https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/compiler/clc/clc_helpers.cpp#L946

@svenvh
Copy link
Member

svenvh commented Oct 6, 2025

For simplicity, can we revert https://reviews.llvm.org/D92231 and don't always define the feature macro in headers?

Not really, reverting that does not solve the underlying issue and would be a step backwards regarding moving header-only extensions out of the Clang internals. If I understand correctly, the underlying issue is that historically the SPIR target had become the equivalent of "enable all known extensions". Perhaps that's something we'd want to reconsider, but that's a separate discussion.

@bader
Copy link
Contributor

bader commented Oct 6, 2025

For simplicity, can we revert https://reviews.llvm.org/D92231 and don't always define the feature macro in headers?

Not really, reverting that does not solve the underlying issue and would be a step backwards regarding moving header-only extensions out of the Clang internals. If I understand correctly, the underlying issue is that historically the SPIR target had become the equivalent of "enable all known extensions". Perhaps that's something we'd want to reconsider, but that's a separate discussion.

My understanding is SPIR target enables all known extensions by default, but I expect that defaults can be overridden.
@svenvh, do I get it right that moving "header-only extensions out of the Clang internals" removes the option to configure the defaults for "header-only extensions"?

@wenju-he
Copy link
Contributor Author

wenju-he commented Oct 6, 2025

do I get it right that moving "header-only extensions out of the Clang internals" removes the option to configure the defaults for "header-only extensions"?

IIUC This should be the main concern. It is fine to enable all extensions by default for SPIR, but moving header-only extensions out of the Clang internals makes it difficult for user to disable specific extensions.

@wenju-he wenju-he merged commit fd73bdd into llvm:main Oct 7, 2025
9 checks passed
@wenju-he wenju-he deleted the __undef___opencl_c_integer_dot_product_input_4x8bit branch October 7, 2025 00:00
@svenvh
Copy link
Member

svenvh commented Oct 7, 2025

@svenvh, do I get it right that moving "header-only extensions out of the Clang internals" removes the option to configure the defaults for "header-only extensions"?

That might not have been the intent, so perhaps this will need some more work adding undef macros similar to those from this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
4 participants