[SYCL] Support non-x86 platforms #2333
Conversation
|
@jeffhammond is this enough to build the runtime library on |
sycl/source/detail/platform_util.cpp
Outdated
| @@ -62,21 +72,49 @@ uint32_t PlatformUtil::getMaxClockFrequency() { | |||
| Buff = Buff.substr(Buff.rfind(' '), Buff.length()); | |||
| Freq *= std::stod(Buff); | |||
| return Freq; | |||
| #else | |||
| #warning Your platform is not supported! | |||
There was a problem hiding this comment.
This is going to produce warning any time when code is compiled for non-x86 platform, right? But patch enables the support of non-x64 platform, so this is a bit confusing.
There was a problem hiding this comment.
The warning is meant to indicate that the specific function is not supported. Should I change the warning? It is very hard to determine the CPU frequency on an ARM platform and the only thing I found requires a Linux 5.x kernel and read access to /sys...
There was a problem hiding this comment.
I see, I am ok to not support this function for ARM.
If someone compiles this for ARM and calls this function 0 is going to be returned, should we also throw runtime error or compile-time warning is enough?
There was a problem hiding this comment.
Currently, this programs throws the following error unconditionally, but I figure that will be removed at some point so I decided to add a placeholder for a non-x86 implementation, for whenever somebody figures out how to get the frequency of an ARM CPU.
throw runtime_error(
"max_clock_frequency parameter is not supported for host device",
PI_INVALID_DEVICE);
Great. This should help closing the oldest open issue #4. Although I'm not sure if @kpet implied adding support for these functions on ARM or just building the library is enough. |
jeffhammond
requested review from
AGindinson,
AlexeySachkov,
AlexeySotkin,
elizabethandrews,
Fznamznon,
kbobrovs,
mdtoguchi,
Pennycook,
premanandrao,
pvchupin,
smaslov-intel,
v-klochkov and
a team
as code owners
tested on AAarch64: - Cavium ThunderX2 with Linux 4.15 - Raspberry Pi 4 with Linux 5.5 Signed-off-by: Jeff R. Hammond <[email protected]>
Signed-off-by: Hammond, Jeff R <[email protected]>
jeffhammond
force-pushed
the
jrhammon-syclrt-platform_util-fixes
branch
from
4971ef0
to
e44b026
Compare
Signed-off-by: Hammond, Jeff R <[email protected]>
bader
requested review from
againull
and removed request for
kbobrovs,
v-klochkov,
AGindinson,
pvchupin,
premanandrao,
mdtoguchi,
smaslov-intel and
elizabethandrews
MSVC does not support #warning compile-time warnings about non-support are not that useful. the code path that generated the warning should not break anything, it just might not be optimal and accurately reflect the underlying platform. Signed-off-by: Hammond, Jeff R <[email protected]>
|
I resolved the remaining outstanding feedback. Is this ready for merging? |
There was a problem hiding this comment.
@jeffhammond, please, fix the build failure on Windows.
D:\buildbot\Product_worker_intel\sycl-win-x64-pr\llvm.src\sycl\source\detail\platform_util.cpp(109): error C2220: warning treated as error - no 'object' file generated
D:\buildbot\Product_worker_intel\sycl-win-x64-pr\llvm.src\sycl\source\detail\platform_util.cpp(109): warning C4189: 'Index': local variable is initialized but not referenced
D:\buildbot\Product_worker_intel\sycl-win-x64-pr\llvm.src\sycl\source\detail\platform_util.cpp(177): error C2065: '_MM_HINT_T0': undeclared identifier
D:\buildbot\Product_worker_intel\sycl-win-x64-pr\llvm.src\sycl\source\detail\platform_util.cpp(177): error C3861: '_mm_prefetch': identifier not found
Signed-off-by: Hammond, Jeff R <[email protected]>
|
@bader I think I fixed it, but since I do not use Windows, I cannot verify. |
|
Let's wait for BuildBot results. |
tested on AAarch64:
Signed-off-by: Jeff R. Hammond [email protected]