[llvm] [Offload] Introduce offload-tblgen and initial new API implementation (PR #108413)

Callum Fare via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 14 04:59:55 PST 2024


callumfare wrote:

Thanks for the review @jhuber6!


>> I'm afraid the code style of this entire stuff will be out of control. Looking at the code, there are at least three different code styles. I don't think it's a good idea to keep as it is.
>
> The public API and internal style can be different like with libomp, but it would definitely help to be consistent.

I've used clang-tidy to keep things as consistent as possible but there are two distinct styles:
- In the public API: `ol_snake_case_t` for types, `olCamelCase` for functions
- In the implementation: Normal LLVM style.

I've kept the public API in that style since it's in line with most other C APIs. If anything the previous conversation was leaning towards making everything snake_case.

If there's any inconsistency beyond that then that's a mistake and I'm happy to correct it.

@shiltian We've previously discussed various aspects of style at the bi-weekly LLVM Offload calls and I think what we have now is as close to a consensus as we could get. I'm not against changing the style again to be purely LLVM style, but I'd want to discuss it as a group again at the next call (December 11th). If you'd like to join that call I'd be happy to have that conversation. In the meantime I would strongly prefer merging this as-is so progress isn't held up. The tablegen tooling means any style or design changes in the future should be fairly painless.

> IMHO, most of the time with this, it roughly means, this is it, and eventually we get so many half-baked stuff.

I understand the concern, but we have many more contributions planned after this. I didn't want the scope of this initial PR to get out of control so have avoided implementing too much of the API initially. In the very short term we want to extend the API to expose all the existing functionality in the libomptarget plugins. Beyond that we want to extend functionality to enable SYCL on top of Offload.

We (Codeplay) plan on having multiple engineers working on this, once this PR is in we should be able to start ramping up effort. We've been attending the bi-weekly Offload meetings and discussing our plans there, and will continue to do so. We're happy to discuss any specific details during a future call.

@jhuber6 I don't have write access. If you're still happy with everything after the above comments would you mind landing the change?

https://github.com/llvm/llvm-project/pull/108413


More information about the llvm-commits mailing list