-
Notifications
You must be signed in to change notification settings - Fork 75
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
Fix parallelLoopPatterns
for the Intel FPGA SYCL back-end
#2456
Fix parallelLoopPatterns
for the Intel FPGA SYCL back-end
#2456
Conversation
I could avoid the new |
We could also make the |
Fixes #2450. |
89deeef
to
3c8249b
Compare
6902658
to
b9416da
Compare
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 request changes but currently do not expect that this is changed. I would like to talk about possible side effects if it is correct what I wrote.
include/alpaka/vec/Vec.hpp
Outdated
typename UDim, | ||
typename UVal, | ||
typename | ||
= std::enable_if_t<UDim::value == TDim::value && std::is_nothrow_convertible_v<std::decay_t<UVal>, TVal>>> |
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 am not sure about this. In alpaka we avoid everywhere implicit casts (if we have it it is IMO a bug).
This constructor opens implicit casts and we are able to convert 64bit values to 32bit values.
This could result in hidden issues due to silent loss of precision.
If I am wrong please correct me!
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.
Mhm, yes, this allows an implicit cast from Vec<Dim3, uint64_t>
to Vec<Dim3, uint32_t>
, in the same way that one can implicitly cast from uint64_t
to uint32_t
.
In genera I'm not fond of all the explicit casts that we need to use with alpaka, but if you don't like to relax this constraint, ok.
I will have to figure out how to fix the test without this.
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, it wasn't complicated :)
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.
By the way, maybe we could still add a conversion constructor, with some requirements:
- if the destination type is larger than the source type (e.g. from
int32_t
toint64_t
), there is not runtime check - otherwise, we explicitly check at runtime that the values can be stored in the destination type without loss of precision
b9416da
to
c770278
Compare
@fwyzard could you please push again to trigger the CI. In case someone is assigned the hzdr bot is crashing an ignoring the job. |
Ah ! |
c770278
to
5bb7a77
Compare
Extend
parallelLoopPatterns
to use 64-bit indices to support more than 2³² elements.Add a dedicated fix for SYCL back-ends, that assume that kernel-related queries (and possibly indices) are limited to
INT_MAX
, unless the-fno-sycl-id-queries-fit-in-int
compiler flag is used.