-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[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
[NFC][libclc] Merge atomic extension built-ins with identical name into a single file #134489
Conversation
…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.
@frasercrmck could you please review, thanks. |
There was a problem hiding this 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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
…y-paste error in atom_and.cl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
…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.
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.