Skip to content

[SYCL] Support non-x86 platforms - #2333

Merged
bader merged 5 commits into
intel:syclfrom
jeffhammond:jrhammon-syclrt-platform_util-fixes
Sep 3, 2020
Merged

bader merged 5 commits into
intel:syclfrom
jeffhammond:jrhammon-syclrt-platform_util-fixes

Conversation

@jeffhammond

Copy link
Copy Markdown
Contributor

tested on AAarch64:

  • Cavium ThunderX2 with Linux 4.15
  • Raspberry Pi 4 with Linux 5.5

Signed-off-by: Jeff R. Hammond [email protected]

@jeffhammond
jeffhammond requested a review from a team as a code owner August 18, 2020 00:30
@jeffhammond
jeffhammond requested a review from againull August 18, 2020 00:30
@bader

bader commented Aug 18, 2020

Copy link
Copy Markdown
Contributor

@jeffhammond is this enough to build the runtime library on AAarch64?

@bader bader changed the title support non-x86 platforms [SYCL] Support non-x86 platforms Aug 18, 2020
@jeffhammond

Copy link
Copy Markdown
Contributor Author

@bader Yes, it's sufficient to build DPC++ on AArch64 and run PRK SYCL programs with the host device. The device execution fails due to #2323 but is independent of this.

Comment thread sycl/source/detail/platform_util.cpp Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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);

Comment thread sycl/source/detail/platform_util.cpp Outdated
Comment thread sycl/source/detail/platform_util.cpp Outdated
@bader bader linked an issue Aug 18, 2020 that may be closed by this pull request
@bader

bader commented Aug 18, 2020

Copy link
Copy Markdown
Contributor

@bader Yes, it's sufficient to build DPC++ on AArch64 and run PRK SYCL programs with the host device. The device execution fails due to #2323 but is independent of this.

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.

Jeff R. Hammond added 2 commits August 23, 2020 09:44
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
jeffhammond force-pushed the jrhammon-syclrt-platform_util-fixes branch from 4971ef0 to e44b026 Compare August 23, 2020 16:44
Signed-off-by: Hammond, Jeff R <[email protected]>
@bader
bader removed request for a team, AlexeySachkov and Pennycook August 23, 2020 17:31
againull
againull previously approved these changes Aug 23, 2020
Comment thread sycl/source/detail/platform_util.cpp Outdated
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]>
@jeffhammond

Copy link
Copy Markdown
Contributor Author

I resolved the remaining outstanding feedback. Is this ready for merging?

@againull
againull self-requested a review September 2, 2020 21:24
@jbrodman
jbrodman self-requested a review September 2, 2020 21:24
jbrodman
jbrodman previously approved these changes Sep 2, 2020
againull
againull previously approved these changes Sep 2, 2020

@bader bader left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@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]>
@jeffhammond
jeffhammond dismissed stale reviews from againull and jbrodman via 76b6e28 September 3, 2020 13:42
@jeffhammond

Copy link
Copy Markdown
Contributor Author

@bader I think I fixed it, but since I do not use Windows, I cannot verify.

@bader

bader commented Sep 3, 2020

Copy link
Copy Markdown
Contributor

Let's wait for BuildBot results.

@againull
againull self-requested a review September 3, 2020 16:51
@bader
bader merged commit 2f632f8 into intel:sycl Sep 3, 2020
@jeffhammond
jeffhammond deleted the jrhammon-syclrt-platform_util-fixes branch September 3, 2020 17:09
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
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.

[SYCL] Support building on ARM

5 participants