Skip to content

[NFC][libclc] Merge atomic extension built-ins with identical name into a single file #134489

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

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Apr 5, 2025

llvm-diff shows there is no change to amdgcn--amdhsa.bc.

Similar to how cl_khr_fp64 and cl_khr_fp16 implementations are put in a same file for math built-ins, this PR do the same to atom_* built-ins.

The main motivation is to prevent that two files with same base name implementats different built-ins. In a follow-up PR, I'd like to relax libclc_configure_lib_source to only compare filename instead of path for overriding, since in our downstream the same category of built-ins, e.g. math, are organized in several different folders.

…to a single file

Similar to how cl_khr_fp64 and cl_khr_fp16 implementations are put in a
same file for math built-ins, this PR do the same to atom_* built-ins.

The main motivation is to prevent that two files with same base name
implementats different built-ins. In a follow-up PR, I'd like to relax
libclc_configure_lib_source to only compare filename instead of path for
overriding, since in our downstream the same category of built-ins, e.g.
math, are organized in several different folders.
@wenju-he
Copy link
Contributor Author

wenju-he commented Apr 5, 2025

@frasercrmck could you please review, thanks.

@frasercrmck frasercrmck added the libclc libclc OpenCL library label Apr 8, 2025
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

Just some general comments, most of which apply to multiple files.

In principle I like the change, though - thank you.

I also think the idea of making overriding based solely on filename is interesting.

@wenju-he wenju-he requested a review from frasercrmck April 9, 2025 01:45
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

LGTM other than the last nit. I can confirm there are no codegen changes to any target that I can observe. Thanks again.

@@ -6,6 +6,17 @@
//
//===----------------------------------------------------------------------===//

// cl_khr_global_int32_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to go back to this but I think really these should all be #if guarded with the appropriate extension: cl_khr_(local|global)_int32_(base|extended)_atomics. It works now because only targets that should build the atomics builtins are including these headers, which isn't the worst thing but it might be good to fix this up since we're here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, you're right that they should be all guarded since they are extensions. Thanks.

@wenju-he wenju-he requested a review from frasercrmck April 10, 2025 00:04
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

Thanks. It seems like a guard mistake in a header has crept into the new headers. Other than that, LGTM.

#include <clc/atomic/atom_decl_int32.inc>
#endif // cl_khr_local_int32_extended_atomics

#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be cl_khr_int64_extended_atomics.

Copy link
Contributor Author

@wenju-he wenju-he Apr 11, 2025

Choose a reason for hiding this comment

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

good eye. done in de7d8b9, thanks. I didn't notice this.

#include <clc/atomic/atom_decl_int32.inc>
#endif // cl_khr_local_int32_extended_atomics

#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

cl_khr_int64_extended_atomics

Copy link
Contributor Author

@wenju-he wenju-he Apr 11, 2025

Choose a reason for hiding this comment

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

done in de7d8b9

#include <clc/atomic/atom_decl_int32.inc>
#endif // cl_khr_local_int32_extended_atomics

#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

cl_khr_int64_extended_atomics

Copy link
Contributor Author

@wenju-he wenju-he Apr 11, 2025

Choose a reason for hiding this comment

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

done in de7d8b9

#include <clc/atomic/atom_decl_int32.inc>
#endif // cl_khr_local_int32_extended_atomics

#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

cl_khr_int64_extended_atomics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in de7d8b9

#include <clc/atomic/atom_decl_int32.inc>
#endif // cl_khr_local_int32_extended_atomics

#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

cl_khr_int64_extended_atomics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in de7d8b9

#endif

/* cl_khr_int64_extended_atomics Extension Functions */
#ifdef cl_khr_int64_base_atomics
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting, this was already wrong.

@wenju-he wenju-he requested a review from frasercrmck April 11, 2025 00:08
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@frasercrmck frasercrmck merged commit cbda72a into llvm:main Apr 14, 2025
9 checks passed
@wenju-he wenju-he deleted the libclc-merge-combine-atomic-extension-functions-into-single-file branch April 14, 2025 13:19
var-const pushed a commit to ldionne/llvm-project that referenced this pull request Apr 17, 2025
…to a single file (llvm#134489)

llvm-diff shows there is no change to amdgcn--amdhsa.bc.

Similar to how cl_khr_fp64 and cl_khr_fp16 implementations are put in a
same file for math built-ins, this PR do the same to atom_* built-ins.

The main motivation is to prevent that two files with same base name
implementats different built-ins. In a follow-up PR, I'd like to relax
libclc_configure_lib_source to only compare filename instead of path for
overriding, since in our downstream the same category of built-ins, e.g.
math, are organized in several different folders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libclc libclc OpenCL library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants