-
Notifications
You must be signed in to change notification settings - Fork 745
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] Added support of rounding modes for non-host devices #1463
[SYCL] Added support of rounding modes for non-host devices #1463
Conversation
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Tagging @Ruyk to take a look at changes at |
sycl/include/CL/sycl/types.hpp
Outdated
template <typename T, typename R, rounding_mode roundingMode> | ||
detail::enable_if_t<!std::is_same<T, R>::value && | ||
(is_int_to_int<T, R>::value && | ||
!(is_int_to_from_uint<T, R>::value) || |
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 line seem cause build error, please double-check
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 forgot to delete it.
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
This connects to @Naghasan 's work |
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
clang/lib/Sema/SPIRVBuiltins.td
Outdated
@@ -726,10 +728,12 @@ foreach IType = [UChar, UShort, UInt, ULong] in { | |||
} | |||
} | |||
|
|||
foreach IType = [Char, Short, Int, Long] in { | |||
foreach IType = [TrueChar, Short, Int, Long] in { |
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.
changing https://github.com/intel/llvm/pull/1463/files#diff-35388f048c1dc54db0a9491da9317c69R290 to char
is the actual fix I believe. This change will trigger unwanted implicit conversions.
Char
is bound to std::int8_t
(always signed char) while TrueChar
is the actual char
type which can be unsigned. I added TrueChar
as a hack for the __spirv_ocl_printf
but I don't think it should be kept.
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.
Ok
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 the link don't seem to work properly, this line
llvm/clang/lib/Sema/SPIRVBuiltins.td
Line 290 in e875fbf
def Char : IntType<"schar", QualType<"SignedCharTy", 0, 1>, 8>; |
schar
to char
so the function name can be built correctly (with e875fbf it is now post-fixing with _Rschar
)
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.
Ok
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
half8{+2.3f, +2.5f, +2.7f, -2.3f, -2.5f, -2.7f, 0.f, 0.f}); | ||
|
||
return 0; | ||
} | ||
} |
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.
Don't see much value in these changes.
Suggest keeping original code.
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.
Only part of this comment was resolved.
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 don't understand, why they are different?
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.
You seem to delete line ending character.
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
// Unsupported: cuda | ||
// The test fails sporadically on cuda. See https://github.com/intel/llvm/issues/1508 for more details. | ||
|
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.
Please, revert changes in this file.
d9b2d52 already disabled the test.
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.
Do I need to do a rebase?
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.
No. CI system does the merge with tip of the sycl branch before testing.
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.
The description doesn't say this, but this patch implements conversion from FP to Int only. What about Int to FP?
I implemented this in the other PR. |
@Naghasan, @erichkeane, @Fznamznon, @turinevgeny, please approve if you don't have any other comments. |
LGTM. |
@turinevgeny, you must approve using GitHub process: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/approving-a-pull-request-with-required-reviews. |
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.
…versioning * origin/sycl: [XPTI][Framework] Reference implementation of the Xpti framework to be used with instrumentation in SYCL (intel#1557) [SYCL] Initial ABI checks implementation (intel#1528) [SYCL] Support connection with multiple plugins (intel#1490) [SYCL] Add a new header file with the reduction class definition (intel#1558) [SYCL] Add test for SYCL kernels with accessor and spec constant (intel#1536) [SYCL][CUDA] Move interop tests (intel#1570) [Driver][SYCL] Remove COFF object format designator for Windows device compiles (intel#1574) [SYCL] Fix conflicting visibility attributes (intel#1571) [SYCL][DOC] Update the SYCL Runtime Interface document with design details (intel#680) [SYCL] Improve image accessors support on a host device (intel#1502) [SYCL] Make queue's non-USM event ownership temporary (intel#1561) [SYCL] Added support of rounding modes for non-host devices (intel#1463) [SYCL] SemaSYCL significant refactoring (intel#1517) [SYCL] Support 0-dim accessor in handler::copy(accessor, accessor) (intel#1551)
Implementing rounding models for cl::sycl::vec type for non host devices.
Signed-off-by: Aleksander Fadeev aleksander.fadeev@intel.com