Skip to content
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

[Libomptarget] Allow the CPU targets to be built without libffi #77495

Merged
merged 1 commit into from Jan 9, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Jan 9, 2024

Summary:
The CPU targets currently rely on libffi to invoke the "kernel"
functions. Previously we would not build these if this dependency was
not found. This patch copies th eapproach used for things like CUDA and
HSA to dynamically load this if it is not found.

The one sketchy thing this does is hard-code the default ABI for the
target. These are normally defined on a per-file basis in the FFI
source, so I had to fish out the expected values. We only use two types,
so ideally we will always be able to use the default ABI.

It's possible we could remove this dependency entirely in the future as
well.

@doru1004
Copy link
Contributor

doru1004 commented Jan 9, 2024

Is there a way to add a test for this? Or is it covered by existing tests?

@doru1004 doru1004 self-requested a review January 9, 2024 16:54
@jhuber6
Copy link
Contributor Author

jhuber6 commented Jan 9, 2024

Is there a way to add a test for this? Or is it covered by existing tests?

It's covered by the existing x64 tests, but you need to force it to use the dlopened path. I did that and it's good on my x64 system. Could check on ppc if really needed, but it would take like a full day to build on Summit.

@doru1004
Copy link
Contributor

doru1004 commented Jan 9, 2024

Is there a way to add a test for this? Or is it covered by existing tests?

It's covered by the existing x64 tests, but you need to force it to use the dlopened path. I did that and it's good on my x64 system. Could check on ppc if really needed, but it would take like a full day to build on Summit.

Does the test suite have a test where it forces the use of the dlopened path?

@jhuber6
Copy link
Contributor Author

jhuber6 commented Jan 9, 2024

Is there a way to add a test for this? Or is it covered by existing tests?

It's covered by the existing x64 tests, but you need to force it to use the dlopened path. I did that and it's good on my x64 system. Could check on ppc if really needed, but it would take like a full day to build on Summit.

Does the test suite have a test where it forces the use of the dlopened path?

No, this is the same as the existing handling for CUDA and HSA. We don't really have a good way to test it in multiple configurations.

Copy link
Contributor

@doru1004 doru1004 left a comment

Choose a reason for hiding this comment

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

LG

@jhuber6 jhuber6 force-pushed the DLOpenFFI branch 2 times, most recently from a681c1a to bebcaec Compare January 9, 2024 18:20
Summary:
The CPU targets currently rely on `libffi` to invoke the "kernel"
functions. Previously we would not build these if this dependency was
not found. This patch copies th eapproach used for things like CUDA and
HSA to dynamically load this if it is not found.

The one sketchy thing this does is hard-code the default ABI for the
target. These are normally defined on a per-file basis in the FFI
source, so I had to fish out the expected values. We only use two types,
so ideally we will always be able to use the default ABI.

It's possible we could remove this dependency entirely in the future as
well.
@jhuber6 jhuber6 merged commit c7c68f1 into llvm:main Jan 9, 2024
4 checks passed
@jplehr
Copy link
Contributor

jplehr commented Jan 10, 2024

After this patch landed our new flang+openmp bot started complaining https://lab.llvm.org/staging/#/builders/140/builds/71

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…#77495)

Summary:
The CPU targets currently rely on `libffi` to invoke the "kernel"
functions. Previously we would not build these if this dependency was
not found. This patch copies th eapproach used for things like CUDA and
HSA to dynamically load this if it is not found.

The one sketchy thing this does is hard-code the default ABI for the
target. These are normally defined on a per-file basis in the FFI
source, so I had to fish out the expected values. We only use two types,
so ideally we will always be able to use the default ABI.

It's possible we could remove this dependency entirely in the future as
well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openmp:libomptarget OpenMP offload runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants