[libspirv] Use clc function in libspirv generic math half_* functions #19779
Conversation
Also delete double/half type implementations, which are not allowed per spec. llvm-diff shows lots of changes in libspirv-amdgcn--amdhsa.bc and libspirv-nvptx64--nvidiacl.bc, due to use of __ocml_* and __nv_* built-ins in clc and libspirv in intel/llvm repo. Based on review comment in llvm/llvm-project#153328, libclc shouldn't use __ocml_*. So bitcode change in this PR is expected.
| #define __FLOAT_ONLY | ||
| #define FUNCTION __spirv_ocl_half_cos | ||
| #define __IMPL_FUNCTION(x) __clc_half_cos | ||
| #define __CLC_BODY <clc/shared/unary_def.inc> |
There was a problem hiding this comment.
I'm not going to let it block this PR, as I don't think it's new to it, but I don't particularly like the naming of these macros. It seems very inconsistent that FUNCTION doesn't use the "reserved" naming with __ prefixed, then we have something as generic as __FLOAT_ONLY. Previously everything was prefixed with __CLC_ to make the association to libclc clear, but now it's only __CLC_BODY, in which the CLC mention now feels redundant given it isn't actually related to the others.
My point is, I prefer the __CLC_ prefix on these, as at the very least it makes their relationship clear. As a reader of this, having __CLC_ on all of these makes it clear to me that they are all intended for the CLC definition generators. I suggest the following names as a follow-up:
| #define __FLOAT_ONLY | |
| #define FUNCTION __spirv_ocl_half_cos | |
| #define __IMPL_FUNCTION(x) __clc_half_cos | |
| #define __CLC_BODY <clc/shared/unary_def.inc> | |
| #define __CLC_GENERATE_FLOAT_ONLY | |
| #define __CLC_FUNCTION __spirv_ocl_half_cos | |
| #define __CLC_IMPL_FUNCTION(x) __clc_half_cos | |
| #define __CLC_BODY <clc/shared/unary_def.inc> |
There was a problem hiding this comment.
My point is, I prefer the
__CLC_prefix on these, as at the very least it makes their relationship clear. As a reader of this, having__CLC_on all of these makes it clear to me that they are all intended for the CLC definition generators. I suggest the following names as a follow-up:
thanks @steffenlarsen for the suggestion. I've created PR llvm/llvm-project#153523 to add _CLC to all macros.
This unifies naming scheme of macros to address review comment intel/llvm#19779 (comment) math constant value macros are not changed, e.g. `#define AU0 -9.86494292470009928597e-03`
|
@intel/llvm-gatekeepers please merge, thanks |
This unifies naming scheme of macros to address review comment intel/llvm#19779 (comment) math constant value macros are not changed, e.g. `#define AU0 -9.86494292470009928597e-03`
…s (#153523) This unifies naming scheme of macros to address review comment intel/llvm#19779 (comment) math constant value macros are not changed, e.g. `#define AU0 -9.86494292470009928597e-03`
3 participants
Also delete double/half type implementations, which are not allowed per spec.
llvm-diff shows lots of changes in libspirv-amdgcn--amdhsa.bc and libspirv-nvptx64--nvidiacl.bc, due to use of _ocml* and _nv* built-ins in clc and libspirv in intel/llvm repo.
Based on review comment in llvm/llvm-project#153328, libclc shouldn't use _ocml*. So bitcode change in this PR is expected.