Skip to content
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

Conversation

fadeeval
Copy link
Contributor

@fadeeval fadeeval commented Apr 2, 2020

Implementing rounding models for cl::sycl::vec type for non host devices.

Signed-off-by: Aleksander Fadeev aleksander.fadeev@intel.com

fadeeval added 18 commits March 27, 2020 16:57
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>
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>
@AlexeySachkov
Copy link
Contributor

Tagging @Ruyk to take a look at changes at SPIRVBuiltins.td

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) ||
Copy link
Contributor

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

Copy link
Contributor Author

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.

sycl/test/basic_tests/vec_convert.cpp Outdated Show resolved Hide resolved
Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
@fadeeval fadeeval requested a review from bader as a code owner April 3, 2020 07:12
fadeeval added 3 commits April 3, 2020 10:17
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>
@fadeeval fadeeval requested a review from erichkeane as a code owner April 3, 2020 08:26
@Ruyk
Copy link
Contributor

Ruyk commented Apr 10, 2020

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>
@@ -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 {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Copy link
Contributor

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

def Char : IntType<"schar", QualType<"SignedCharTy", 0, 1>, 8>;
needs to changed from schar to char so the function name can be built correctly (with e875fbf it is now post-fixing with _Rschar)

Copy link
Contributor Author

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>
Comment on lines 153 to 155
half8{+2.3f, +2.5f, +2.7f, -2.3f, -2.5f, -2.7f, 0.f, 0.f});

return 0;
}
}
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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>
Comment on lines 7 to 9
// Unsupported: cuda
// The test fails sporadically on cuda. See https://github.com/intel/llvm/issues/1508 for more details.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Signed-off-by: Aleksander Fadeev <aleksander.fadeev@intel.com>
@fadeeval fadeeval requested a review from bader April 16, 2020 07:03
Copy link
Contributor

@bader bader left a 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?

@fadeeval
Copy link
Contributor Author

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.

@bader
Copy link
Contributor

bader commented Apr 22, 2020

@Naghasan, @erichkeane, @Fznamznon, @turinevgeny, please approve if you don't have any other comments.

@turinevgeny
Copy link
Contributor

LGTM.

@bader
Copy link
Contributor

bader commented Apr 22, 2020

Copy link
Contributor

@turinevgeny turinevgeny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@bader bader merged commit 7e3cca4 into intel:sycl Apr 22, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Apr 29, 2020
…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)
@fadeeval fadeeval deleted the private/fadeeval/support_rounding_models_for_non-host_devices branch May 21, 2020 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants