[SYCL] Check for nan/inf in stream class #522
Conversation
| @@ -83,7 +83,7 @@ static inline std::string codeToString(cl_int code){ | |||
| #endif | |||
|
|
|||
| #if __has_attribute(always_inline) | |||
| #define ALWAYS_INLINE __attribute__((always_inline)) | |||
| #define ALWAYS_INLINE __attribute__((always_inline)) inline | |||
There was a problem hiding this comment.
How this modification related to the adding check for nan values?
There was a problem hiding this comment.
I use sycl builtins to check for nan/inf values. Usage of these builtins causes warnings. Purpose of this change is to fix these warnings.
There was a problem hiding this comment.
You are changing the define semantics w/o updating the name!
Can you just apply inline keyword to specific built-ins you are using?
Also I'm not sure I follow what warning is emitted, so it's hard to say what is the right fix.
There was a problem hiding this comment.
Moved inline keyword from macro to builtin declaration.
Short description of the problem:
According to always_inline attribute description in gcc's documentation https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html:
For functions *declared inline*, this attribute inlines the function independent of any restrictions that otherwise apply to inlining.
This means that if declaration is not marked inline then warning will be generated.
Problem could be reproduced in the following test case:
#include <CL/sycl.hpp>
int main() {
return 0;
}
When compiling with gcc 5.4.0:
g++ -std=c++11 -I$PATH_TO_SYCL/sycl/include test.cpp -c
gives the bunch of warnings:
"sycl/include/CL/sycl/detail/builtins.hpp:32:19: warning: always_inline function might not be inlinable [-Wattributes]"
If you think that I shouldn't fix this in this patch or my fix doesn't look good then I can create issue and assign it to the author.
| if (Exp16 == 0x1f) { | ||
| if (Sign) | ||
| return append(Buf, "-inf"); | ||
| else |
There was a problem hiding this comment.
Please, do not put else after return.
| else if (isinf(Val)) { | ||
| if (signbit(Val)) | ||
| return append(Buf, "-inf"); | ||
| else |
There was a problem hiding this comment.
Please, do not put else after return.
againull
force-pushed
the
stream_nan_inf
branch
from
84de49c
to
a61f068
Compare
| checkForInfNan(char *Buf, T Val) { | ||
| if (isnan(Val)) | ||
| return append(Buf, "nan"); | ||
| else if (isinf(Val)) { |
There was a problem hiding this comment.
No else after return, please.
Signed-off-by: Artur Gainullin <artur.gainullin@intel.com>
againull
force-pushed
the
stream_nan_inf
branch
from
a61f068
to
1371320
Compare
intel#522 removed the propagation of the ImportedHostPtr argument from _pi_buffer to the _pi_mem superclass. This seems like an unintended change and this commit re-adds the propagation. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
#522 removed the propagation of the ImportedHostPtr argument from _pi_buffer to the _pi_mem superclass. This seems like an unintended change and this commit re-adds the propagation. Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Signed-off-by: Artur Gainullin artur.gainullin@intel.com