[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. |
| @@ -6,6 +6,17 @@ | |||
| // | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| // cl_khr_global_int32_base_atomics | |||
There was a problem hiding this comment.
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.
done, you're right that they should be all guarded since they are extensions. Thanks.
| #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.
I think this should be cl_khr_int64_extended_atomics.
There was a problem hiding this comment.
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.
cl_khr_int64_extended_atomics
| #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.
cl_khr_int64_extended_atomics
| #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.
cl_khr_int64_extended_atomics
| #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.
cl_khr_int64_extended_atomics
| #endif | ||
|
|
||
| /* cl_khr_int64_extended_atomics Extension Functions */ | ||
| #ifdef cl_khr_int64_base_atomics |
There was a problem hiding this comment.
Ah interesting, this was already wrong.
…y-paste error in atom_and.cl
wenju-he
deleted the
libclc-merge-combine-atomic-extension-functions-into-single-file
branch
…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.