Skip to content

Add unions for ICD2 tags in cl_icd_dispatch #280

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

Merged
merged 2 commits into from
Jun 17, 2025

Conversation

karolherbst
Copy link
Contributor

This requires C11/C++11 as support for anonymous unions got added there.

I need this, because otherwise rustc doesn't let me use a constant in place of a function pointer. But I think being more explicit here would make the code a bit cleaner for loader implementors.

@karolherbst
Copy link
Contributor Author

rust bug for reference: rust-lang/rust#141660

@Kerilk
Copy link
Contributor

Kerilk commented May 27, 2025

I love the suggestion, but there is another instance of anonymous union where guards exist:

OpenCL-Headers/CL/cl.h

Lines 137 to 167 in 1ce4f1c

#if defined(__GNUC__)
__extension__ /* Prevents warnings about anonymous union in -pedantic builds */
#endif
#if defined(_MSC_VER) && !defined(__STDC__)
#pragma warning( push )
#pragma warning( disable : 4201 ) /* Prevents warning about nameless struct/union in /W4 builds */
#endif
#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wc11-extensions" /* Prevents warning about nameless union being C11 extension*/
#endif
#if defined(_MSC_VER) && defined(__STDC__)
/* Anonymous unions are not supported in /Za builds */
#else
union {
#endif
#endif
cl_mem buffer;
#ifdef CL_VERSION_2_0
#if defined(_MSC_VER) && defined(__STDC__)
/* Anonymous unions are not supported in /Za builds */
#else
cl_mem mem_object;
};
#endif
#if defined(_MSC_VER) && !defined(__STDC__)
#pragma warning( pop )
#endif
#ifdef __clang__
#pragma clang diagnostic pop
#endif

I think we would need something similar to enable this.

@karolherbst
Copy link
Contributor Author

mhhh.. not a big fan of replicating the same code block, but maybe this can be moved into a macro or some sorts. Will play around with it later.

We already have helper macros in cl_platform.h we can use instead of the
current mess.
@karolherbst
Copy link
Contributor Author

Turns out there were already a bit of helper macros in cl_platform for anonymous structs we can also reuse for unions. Cleaned up code in cl.h and used the more streamlined stuff in cl_icd.h as well

@karolherbst karolherbst force-pushed the icd_union branch 2 times, most recently from 434c119 to e245a57 Compare May 28, 2025 12:18
I need this, because otherwise rustc doesn't let me use a constant in
place of a function pointer. But I think being more explicit here would
make the code a bit cleaner for loader implementors.
@Kerilk
Copy link
Contributor

Kerilk commented May 28, 2025

@Kerilk Kerilk requested a review from bashbaug May 28, 2025 14:09
Copy link
Contributor

@bashbaug bashbaug 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'm a little nervous about losing some compiler-specific warning suppression, but if the macros are working well for the other anonymous structs, they should work fine here, too.

@karolherbst
Copy link
Contributor Author

LGTM

I'm a little nervous about losing some compiler-specific warning suppression, but if the macros are working well for the other anonymous structs, they should work fine here, too.

yeah.. I've checked clang docs and it uses __extension__ (__CL_ANON_STRUCT__ expands to it) similar to gcc, so I don't think those changes are changing anything in this regards.

The only part I'm a bit more concerned about is MSVC and the pop/push of the warning is a bit global. Maybe they have a similar mechanism like __extension__ we could use instead?

Kerilk added a commit to Kerilk/OpenCL-ICD-Loader that referenced this pull request May 28, 2025
@Kerilk
Copy link
Contributor

Kerilk commented May 28, 2025

We can rerun the CI in KhronosGroup/OpenCL-ICD-Loader#252 once this one is merged as an additional check.
(Works fine on my machine.™)

@bashbaug
Copy link
Contributor

Merging as discussed in the June 17th teleconference.

@bashbaug bashbaug merged commit b79b358 into KhronosGroup:main Jun 17, 2025
74 checks passed
Kerilk added a commit to Kerilk/OpenCL-ICD-Loader that referenced this pull request Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants