fix fp16 with msvc #333
fix fp16 with msvc #333
Conversation
| #include "./base.h" | ||
|
|
||
| #if MSHADOW_USE_F16C | ||
| #include <x86intrin.h> |
There was a problem hiding this comment.
Users also report that this breaks android build
There was a problem hiding this comment.
wont amalgamation test in mxnet ci test for this ?
There was a problem hiding this comment.
Could you point me to the issue for this if it exists?
There was a problem hiding this comment.
Android build should be using USE_F16C=0 as this feature wouldn't be supported for that hardware. Similar to how they need to turn off USE_SSE
mshadow/half.h
Outdated
| #if MSHADOW_USE_F16C | ||
| #include <x86intrin.h> | ||
| #if defined(_MSC_VER) | ||
| #include <intrin.h> |
There was a problem hiding this comment.
You had mentioned in your PR description that msvc doesn't have intrin.h. If so why would we want to include this? Especially given that below _cvtsh_ss is turned off for MSVC?
There was a problem hiding this comment.
I wrote it wrong. It's only "intrin.h", no "x86intrin.h""
szha
left a comment
szha
left a comment
There was a problem hiding this comment.
Approving for the clang related change. BTW was the problem isolated to OSX clang? Or were all related versions affected?
|
@szha now is only OSX.Maybe it should be changed to |
|
@yajiedesign yes, it's probably a clang problem. https://reviews.llvm.org/D16177 |
|
@piiswrong can this be merged ? |
5 participants
msvc not inculde x86intrin.h and _cvtsh_ss