Skip to content

[Libomptarget] Consolidate CPU offloading into 'host' directory #86014

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 1 commit into from
Mar 20, 2024

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Mar 20, 2024

Summary:
All of these CPU targets use the same underlying implementation. We
should consolidate them into a single target to make it easier to update
this to a static library based approach. I have decided to call this the
'host' target so it can be given a single name. We still only build
these if the system processor matches and we are on Linux.

Summary:
All of these CPU targets use the same underlying implementation. We
should consolidate them into a single target to make it easier to update
this to a static library based approach. I have decided to call this the
'host' target so it can be given a single name. We still only build
these if the system processor matches and we are on Linux.
Copy link
Member

@jdoerfert jdoerfert 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 merged commit a7d5f73 into llvm:main Mar 20, 2024
"aarch64-unknown-linux-gnu" "aarch64-unknown-linux-gnu-LTO")
set(LIBOMPTARGET_SYSTEM_TARGETS "${LIBOMPTARGET_SYSTEM_TARGETS}" PARENT_SCOPE)
elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "s390x$")
target_compile_definitions(omptarget.rtl.${machine} TARGET_ELF_ID=EM_S390)
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing "PRIVATE" on this line, which caused the build to break on SystemZ. Fixed in cb07194.

chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…#86014)

Summary:
All of these CPU targets use the same underlying implementation. We
should consolidate them into a single target to make it easier to update
this to a static library based approach. I have decided to call this the
'host' target so it can be given a single name. We still only build
these if the system processor matches and we are on Linux.
Comment on lines +82 to +86
target_compile_definitions(omptarget.rtl.${machine} PRIVATE TARGET_ELF_ID=EM_PPC64)
target_compile_definitions(omptarget.rtl.${machine} PRIVATE
LIBOMPTARGET_NEXTGEN_GENERIC_PLUGIN_TRIPLE="powerpc64-ibm-linux-gnu")
list(APPEND LIBOMPTARGET_SYSTEM_TARGETS
"powerpc64-ibm-linux-gnu" "powerpc64-ibm-linux-gnu-LTO")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is incorrect, because it is specifying a big-endian triple (powerpc64-ibm-linux-gnu) when the host is little-endian (ppc64le). The ppc64le and ppc64 $CMAKE_SYSTEM_PROCESSOR values need to be handled separately.

cc @tuliom

Copy link
Contributor

@tuliom tuliom Apr 15, 2024

Choose a reason for hiding this comment

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

@tstellar Agreed. It's worth mentioning that using powerpc64le in triples is more frequent than ppc64le.
This is the first time I see -ibm being used for a ppc64*-linux system.
Is the usage of -ibm really necessary? And what it means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made #88773. Using the vendor field doesn't really do anything for these triples. It's the same as x86_64-pc-linux-gnu which doesn't make a difference except for runtime directory stuff.

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.

6 participants