Guard libdevice code with __SPIR__. #1672
Conversation
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Signed-off-by: Vyacheslav Zakharin <[email protected]>
Signed-off-by: Vyacheslav Zakharin <[email protected]>
There was a problem hiding this comment.
LGTM. You might want to add a clang regression test for __SPIR__ definition.
@asavonic, @Fznamznon, @erichkeane, ping.
libdevice/device.h
Outdated
| // required by the wrapper libraries. | ||
| #include "spirv_vars.hpp" | ||
| #else // __SPIR__ | ||
| #define __SPIR_TARGET_ONLY__ 0 |
There was a problem hiding this comment.
Is this macro needed for non-spir targets? Can it be defined only for spir?
There was a problem hiding this comment.
One more question: why can't just use __SPIR__?
There was a problem hiding this comment.
Is this macro needed for non-spir targets? Can it be defined only for spir?
I want to make sure that this header file is the only place to control it, so I want to have this macro defined in all compilations. If it is (accidentally) redefined, then the build will break making the issue evident. This is a devicelib internal macro that I want to have full control over.
There was a problem hiding this comment.
One more question: why can't just use
__SPIR__?
It makes any future manipulations with the implementation guard simpler. Not that I expect any further modifications, but I already did it once (when I removed __SYCL_DEVICE_ONLY__), and it required changes in all files.
There was a problem hiding this comment.
If it is (accidentally) redefined, then the build will break making the issue evident.
I don't know if it is a real issue. Compiler does not define this macro, users do not define it either. Header defines it in a single place surrounded by include guards.
Macros such as this are usually not defined on "other" platforms. This way __SYCL_DEVICE_ONLY__ is not defined for host compilation (as opposed to be defined to 0) and __X86_64__ is not defined for arm.
There was a problem hiding this comment.
I am not worried about users. I am worried about developers :) __SPIR_TARGET_ONLY__ kind of invades onto FE's territory. If at any point it is end up being defined for whatever reason, I want to be aware of it.
If you do not like #define __SPIR_TARGET_ONLY__ 0, I can replace it with #undef __SPIR_TARGET_ONLY__ in the else clause, but I want to have the controlled behavior in all compilations.
There was a problem hiding this comment.
Why not choose a unique prefix, such as __DEVICELIB_DEVICE_ONLY__?
There was a problem hiding this comment.
There is no such thing as unique name, unless it is already defined by FE.
I think we waste too much time discussing this. How about I get rid of __SPIR_TARGET_ONLY__ and use just __SPIR__ instead? I believe everybody agrees with that, am I right?
Signed-off-by: Vyacheslav Zakharin <[email protected]>
|
@erichkeane - ping |
I suggest submitting CFE stuff to llorg as well. |
Signed-off-by: Vyacheslav Zakharin <[email protected]>
I agree. Will do. |
|
@asavonic - ping |
|
CFE changes uploaded to llorg: https://reviews.llvm.org/D80655 |
The latest changes got interfered with the patch that fixes blocks ordering. Like this the checks actually make more sense. Signed-off-by: Sidorov, Dmitry <[email protected]> Signed-off-by: Sidorov, Dmitry <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@3e6bf42
libdeviceprovides standard libc/libm functionality forSPIRdevices. It is notSYCLspecific, so the use ofSYCLspecific definitions should be minimal. In most cases we can use__SPIR__macro instead of__SYCL_DEVICE_ONLY__.Signed-off-by: Vyacheslav Zakharin [email protected]