-
Notifications
You must be signed in to change notification settings - Fork 744
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
[SYCL-PTX] Add math builtins categories #1990
Conversation
Signed-off-by: Victor Lomuller <[email protected]> Co-authored-by: David Wood <[email protected]>
64e9720
to
35fbd3f
Compare
@@ -8,7 +8,7 @@ | |||
|
|||
#include <spirv/spirv.h> | |||
|
|||
#include "../../lib/clcmacro.h" | |||
#include <clcmacro.h> |
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.
This probably should belong to other PR.
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 assume these "common" built-ins are used in "math" built-ins implementation, right?
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.
This probably should belong to other PR.
That's a change that relates to the refactoring prior to this patch. This wasn't part of it as it directly conflicted with the PR #1832. This avoided a dependency.
The header file included is the same, ../../lib/clcmacro.h
is a link to the one in include
. So there is no functional change, just consistency.
I assume these "common" built-ins are used in "math" built-ins implementation, right?
Some common
builtins are indeed used by the math
one.
*/ | ||
|
||
#include <clc/clc.h> | ||
//===----------------------------------------------------------------------===// |
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.
Hm... shouldn't we keep old AMD copyright header?
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 can revert if you prefer.
Technically, the LLVM relicensed all sub-projects under this license, but this one never updated the headers. But I'm no lawyer.
Co-authored-by: Alexey Bader <[email protected]>
Co-authored-by: Alexey Bader <[email protected]>
Co-authored-by: Alexey Bader <[email protected]>
Co-authored-by: Alexey Bader <[email protected]>
…tel#1990) Add the library `BINARY_DIR` to `LD_LIBRARY_PATH` to ensure that the freshly built `libLLVMSPIRVLib.so` is tested. Otherwise, llvm-spirv spawned by the test suite may use the previously installed `libLLVMSPIRVLib.so`. I have noticed the problem after rebuilding LLVM with `-DLLVM_ENABLE_ASSSERTIONS=ON`. This meant that the previous version of `libLLVMSPIRVLib.so` now crashed, effectively causing the test suite to fail incorrectly. Signed-off-by: Michał Górny <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@ba965cd
The patch adds the generic SPIR-V implementation for the math builtins (so nvptx specific implementation yet).
Signed-off-by: Victor Lomuller [email protected]
Co-authored-by: David Wood [email protected]