Skip to content

Reland #118503: [Offload] Introduce offload-tblgen and initial new API implementation #118614

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

Merged
merged 28 commits into from
Dec 5, 2024

Conversation

callumfare
Copy link
Contributor

Reland #118503. Added a fix for builds with -DBUILD_SHARED_LIBS=ON (see last commit). Otherwise the changes are identical.


New API

Previous discussions at the LLVM/Offload meeting have brought up the need for a new API for exposing the functionality of the plugins. This change introduces a very small subset of a new API, which is primarily for testing the offload tooling and demonstrating how a new API can fit into the existing code base without being too disruptive. Exact designs for these entry points and future additions can be worked out over time.

The new API does however introduce the bare minimum functionality to implement device discovery for Unified Runtime and SYCL. This means that the urinfo and sycl-ls tools can be used on top of Offload. A (rough) implementation of a Unified Runtime adapter (aka plugin) for Offload is available here. Our intention is to maintain this and use it to implement and test Offload API changes with SYCL.

Demoing the new API

# From the runtime build directory
$ ninja LibomptUnitTests
$ OFFLOAD_TRACE=1 ./offload/unittests/OffloadAPI/offload.unittests 

Open questions and future work

  • Only some of the available device info is exposed, and not all the possible device queries needed for SYCL are implemented by the plugins. A sensible next step would be to refactor and extend the existing device info queries in the plugins. The existing info queries are all strings, but the new API introduces the ability to return any arbitrary type.
  • It may be sensible at some point for the plugins to implement the new API directly, and the higher level code on top of it could be made generic, but this is more of a long-term possibility.

@callumfare
Copy link
Contributor Author

callumfare commented Dec 4, 2024

Hi @jplehr, I've added a fix for the shared libs build, see the last commit. It's unfortunate that there's a dependency on FrontendOpenMP, but I think there would need to be a big refactor to remove all openmp dependencies from offload.

I've also verified a standalone build, including the check-offload and LibomptUnitTests targets, to make sure this doesn't cause a regression after #118173.

Let me know if there are any issues

@jplehr jplehr self-requested a review December 4, 2024 14:25
Copy link
Contributor

@jplehr jplehr left a comment

Choose a reason for hiding this comment

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

I ran this through different build configs and they worked fine. Let's give it another try, I guess.

@jplehr jplehr merged commit fd3907c into llvm:main Dec 5, 2024
6 checks passed
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Dec 5, 2024
breaks build OMPT related?
ld.lld: error: undefined symbol: llvm::omp::target::ompt::enableDeviceTracing(int)
b877a533a6d1 (HEAD -> amd-staging) Revert "Reland llvm#118503: [Offload] Introduce offload-tblgen and initial new API implementation (llvm#118614)"

Change-Id: I1f4cb8e06ca625c2a539348d897e5436d314fc0a
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Dec 7, 2024
…w API implementation (llvm#118614)

Reland llvm#118503. Added a fix for builds with `-DBUILD_SHARED_LIBS=ON`
(see last commit). Otherwise the changes are identical.

Disables until OMPT adapts to this.

Previous discussions at the LLVM/Offload meeting have brought up the
need for a new API for exposing the functionality of the plugins. This
change introduces a very small subset of a new API, which is primarily
for testing the offload tooling and demonstrating how a new API can fit
into the existing code base without being too disruptive. Exact designs
for these entry points and future additions can be worked out over time.

The new API does however introduce the bare minimum functionality to
implement device discovery for Unified Runtime and SYCL. This means that
the `urinfo` and `sycl-ls` tools can be used on top of Offload. A
(rough) implementation of a Unified Runtime adapter (aka plugin) for
Offload is available
[here](https://github.com/callumfare/unified-runtime/tree/offload_adapter).
Our intention is to maintain this and use it to implement and test
Offload API changes with SYCL.

```sh
$ ninja LibomptUnitTests
$ OFFLOAD_TRACE=1 ./offload/unittests/OffloadAPI/offload.unittests
```

* Only some of the available device info is exposed, and not all the
possible device queries needed for SYCL are implemented by the plugins.
A sensible next step would be to refactor and extend the existing device
info queries in the plugins. The existing info queries are all strings,
but the new API introduces the ability to return any arbitrary type.
* It may be sensible at some point for the plugins to implement the new
API directly, and the higher level code on top of it could be made
generic, but this is more of a long-term possibility.

Change-Id: I355ac2cc4122d8023a88bc7f427174e16dfa76b9
@nikic
Copy link
Contributor

nikic commented Dec 9, 2024

Something problematic about how offload-tblgen is set up is that all of offload will (typically) be built as a runtime, while offload-tblgen is a host binary and depends on a host library. The compiler configuration used for the host compilation and the runtime build may be substantially different.

In our case it breaks because both compilers use different libstdc++ versions (more by accident than intention), but I'd expect this to also break if the built clang used for runtime builds is only configured for cross-compilation.

@callumfare
Copy link
Contributor Author

Hi @nikic, for offload-tblgen I mostly copied how mlir-tblgen is set up but I didn't account for the differences in building the project as a runtime. I'll look into this asap but if you any pointers on how to resolve it I'd appreciate the help

@callumfare
Copy link
Contributor Author

@nikic Do you see the same problem with building libomptarget (also part of the offload runtime)? I've had a look and it seems to also pull in the host-built libLLVMSupport.a etc. Either way I can see the issue but I'm wondering if there's an existing workaround that I'm not aware of.

I don't think the cross-compilation case is broken because add_tablegen respects LLVM_USE_HOST_TOOLS although I haven't verified that.

@nikic
Copy link
Contributor

nikic commented Dec 11, 2024

@nikic Do you see the same problem with building libomptarget (also part of the offload runtime)? I've had a look and it seems to also pull in the host-built libLLVMSupport.a etc.

Interesting question! It does seem to be in a similar situation (though not quite, as this is at least an actual runtime). I think the relevant difference ends up being that libomptarget can link libLLVM.so, while offload-tblgen (like any tblgen) links against libLLVMSupport.a statically.

Either way I can see the issue but I'm wondering if there's an existing workaround that I'm not aware of.

Hm, I don't really see anything simple that can be done here. As far as I understand the entire offload build is going to be in a nested cmake invocation, so we can't really split it into a runtime and non-runtime part. Maybe there's just nothing that can be done here. (I've worked around the specific problem that we hit already...)

milkice233 pushed a commit to fedora-riscv/llvm that referenced this pull request Jan 22, 2025
Package the new libLLVMOffload.so and /usr/include/offload headers
introduced in llvm/llvm-project#118614.
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.

3 participants