-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Offload] Change ELF machine type for SPIR-V OpenMP image #159623
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
Conversation
@llvm/pr-subscribers-clang Author: Nick Sarnie (sarnex) ChangesThis needs to match the runtime plugin (currently in PR here), and the plugin uses Full diff: https://github.com/llvm/llvm-project/pull/159623.diff 2 Files Affected:
diff --git a/clang/test/Tooling/clang-linker-wrapper-spirv-elf.cpp b/clang/test/Tooling/clang-linker-wrapper-spirv-elf.cpp
index af98904677283..1bf4572ee566c 100644
--- a/clang/test/Tooling/clang-linker-wrapper-spirv-elf.cpp
+++ b/clang/test/Tooling/clang-linker-wrapper-spirv-elf.cpp
@@ -7,9 +7,12 @@
// RUN: %clangxx -fopenmp -fopenmp-targets=spirv64-intel -nogpulib -c -o %t_clang-linker-wrapper-spirv-elf.o %s
// RUN: not clang-linker-wrapper -o a.out %t_clang-linker-wrapper-spirv-elf.o --save-temps --linker-path=ld
// RUN: clang-offload-packager --image=triple=spirv64-intel,kind=openmp,file=%t.elf %t_tmp/a.out.openmp.image.wrapper.o
+// RUN: llvm-readelf -h %t.elf | FileCheck -check-prefix=CHECK-MACHINE %s
// RUN: llvm-readelf -t %t.elf | FileCheck -check-prefix=CHECK-SECTION %s
// RUN: llvm-readelf -n %t.elf | FileCheck -check-prefix=CHECK-NOTES %s
+// CHECK-MACHINE: Machine: 8086
+
// CHECK-SECTION: .note.inteloneompoffload
// CHECK-SECTION: __openmp_offload_spirv_0
diff --git a/llvm/lib/Frontend/Offloading/Utility.cpp b/llvm/lib/Frontend/Offloading/Utility.cpp
index 5dcc16d23004c..7cb83987f36d2 100644
--- a/llvm/lib/Frontend/Offloading/Utility.cpp
+++ b/llvm/lib/Frontend/Offloading/Utility.cpp
@@ -423,9 +423,10 @@ Error offloading::intel::containerizeOpenMPSPIRVImage(
Header.Class = ELF::ELFCLASS64;
Header.Data = ELF::ELFDATA2LSB;
Header.Type = ELF::ET_DYN;
- // Use an existing Intel machine type as there is not one specifically for
- // Intel GPUs.
- Header.Machine = ELF::EM_IA_64;
+ // Use a fake machine type as there is not one specifically for
+ // Intel GPUs, the associated runtime plugin is looking for
+ // this value.
+ Header.Machine = 0x8086;
// Create a section with notes.
ELFYAML::NoteSection Section{};
|
@adurang Can't add you as a reviewer in the UI but I would appreciate your review, should be easy :) |
// Use a fake machine type as there is not one specifically for | ||
// Intel GPUs, the associated runtime plugin is looking for | ||
// this value. | ||
Header.Machine = 0x8086; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making one up is kind of scary, I'd prefer adding it to the ELF header as a vendor specific version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was worried we'd have to officially add it to ELF spec as per this link and then update all userspace tools, if you're okay with us only changing the LLVM ELF tools I'd be happy to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd obviously be best to go the official route if this is going to be used in a real context, if it's just a test then I guess we can keep it as a constant. But I would definitely question why Intel hasn't already defined this if it's their official ELF format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The complication here is that we're jitting, so the device image is an ELF that just contains a spirv image unlike what we have for AMD where's its a real device binary in ELF format. when we compile the spv with the gpu driver we do get an ELF that contains actual device code, let me see what machine code the final gpu executable is using and i guess we can just use that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow we already have one
https://groups.google.com/g/generic-abi/c/ofBevXA48dM?pli=1
#define EM_INTELGT 205 /* Intel Graphics Technology */
definitely going to use this, my bad i didn't know we had one, thanks for questioning this lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EM_INTEL205 = 205, // Reserved by Intel
Time to rename this I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, making a separate PR for that right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully the latest version of this PR is less scary
Signed-off-by: Sarnie, Nick <[email protected]>
Signed-off-by: Sarnie, Nick <[email protected]>
This needs to match the runtime plugin (currently in PR here), and use the recently-added
INTELGT
machine type which is correct for Intel GPU images.