[Libomptarget] Remove __tgt_image_info and use the ELF directly #75720
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhuber6
requested review from
Artem-B,
jdoerfert,
JonChesterfield,
jplehr,
saiislam,
shiltian and
yxsamliu
|
@llvm/pr-subscribers-backend-amdgpu Author: Joseph Huber (jhuber6) ChangesSummary: The main desire to do this was to reorganize everything into the ELF This patch also adds a lot more sanity checks for whether or not the ELF Patch is 34.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75720.diff 15 Files Affected:
diff --git a/openmp/libomptarget/include/DeviceImage.h b/openmp/libomptarget/include/DeviceImage.h
index 465bf970ef17fe..63b4b6d14e0ef4 100644
--- a/openmp/libomptarget/include/DeviceImage.h
+++ b/openmp/libomptarget/include/DeviceImage.h
@@ -30,20 +30,13 @@ class DeviceImageTy {
__tgt_bin_desc *BinaryDesc;
__tgt_device_image Image;
- __tgt_image_info ImageInfo;
public:
DeviceImageTy(__tgt_bin_desc &BinaryDesc, __tgt_device_image &Image);
__tgt_device_image &getExecutableImage() { return Image; }
- __tgt_image_info &getImageInfo() { return ImageInfo; }
__tgt_bin_desc &getBinaryDesc() { return *BinaryDesc; }
- llvm::StringRef
- getArch(llvm::StringRef DefaultArch = llvm::StringRef()) const {
- return ImageInfo.Arch ? ImageInfo.Arch : DefaultArch;
- }
-
auto entries() { return llvm::make_pointee_range(OffloadEntries); }
};
diff --git a/openmp/libomptarget/include/Shared/APITypes.h b/openmp/libomptarget/include/Shared/APITypes.h
index 8e2aee2deb2957..763a22f0a5e863 100644
--- a/openmp/libomptarget/include/Shared/APITypes.h
+++ b/openmp/libomptarget/include/Shared/APITypes.h
@@ -46,11 +46,6 @@ struct __tgt_device_info {
void *Device = nullptr;
};
-/// This struct contains information about a given image.
-struct __tgt_image_info {
- const char *Arch;
-};
-
/// This struct is a record of all the host code that may be offloaded to a
/// target.
struct __tgt_bin_desc {
diff --git a/openmp/libomptarget/include/Shared/PluginAPI.h b/openmp/libomptarget/include/Shared/PluginAPI.h
index 41d1908da21532..c6aacf4ce2124b 100644
--- a/openmp/libomptarget/include/Shared/PluginAPI.h
+++ b/openmp/libomptarget/include/Shared/PluginAPI.h
@@ -35,12 +35,6 @@ int32_t __tgt_rtl_number_of_devices(void);
// having to load the library, which can be expensive.
int32_t __tgt_rtl_is_valid_binary(__tgt_device_image *Image);
-// This provides the same functionality as __tgt_rtl_is_valid_binary except we
-// also use additional information to determine if the image is valid. This
-// allows us to determine if an image has a compatible architecture.
-int32_t __tgt_rtl_is_valid_binary_info(__tgt_device_image *Image,
- __tgt_image_info *Info);
-
// Return an integer other than zero if the data can be exchaned from SrcDevId
// to DstDevId. If it is data exchangable, the device plugin should provide
// function to move data from source device to destination device directly.
diff --git a/openmp/libomptarget/include/Shared/PluginAPI.inc b/openmp/libomptarget/include/Shared/PluginAPI.inc
index 0949e4e593ddeb..25ebe7d437f9d1 100644
--- a/openmp/libomptarget/include/Shared/PluginAPI.inc
+++ b/openmp/libomptarget/include/Shared/PluginAPI.inc
@@ -15,7 +15,6 @@
PLUGIN_API_HANDLE(init_plugin, true);
PLUGIN_API_HANDLE(is_valid_binary, true);
-PLUGIN_API_HANDLE(is_valid_binary_info, false);
PLUGIN_API_HANDLE(is_data_exchangable, false);
PLUGIN_API_HANDLE(number_of_devices, true);
PLUGIN_API_HANDLE(init_device, true);
diff --git a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
index 0ffdabe5bcd420..347d7529e947b3 100644
--- a/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
+++ b/openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp
@@ -23,6 +23,7 @@
#include "Shared/Debug.h"
#include "Shared/Environment.h"
#include "Shared/Utils.h"
+#include "Utils/ELF.h"
#include "GlobalHandler.h"
#include "OpenMP/OMPT/Callback.h"
@@ -3015,7 +3016,16 @@ struct AMDGPUPluginTy final : public GenericPluginTy {
uint16_t getMagicElfBits() const override { return ELF::EM_AMDGPU; }
/// Check whether the image is compatible with an AMDGPU device.
- Expected<bool> isImageCompatible(__tgt_image_info *Info) const override {
+ Expected<bool> isELFCompatible(StringRef Image) const override {
+ // Get the associated architecture from the ELF.
+ auto ProcessorOrErr = ::utils::elf::getProcessor(Image);
+ if (!ProcessorOrErr)
+ return ProcessorOrErr.takeError();
+
+ auto FlagsOrErr = ::utils::elf::getFlags(Image);
+ if (!FlagsOrErr)
+ return FlagsOrErr.takeError();
+
for (hsa_agent_t Agent : KernelAgents) {
std::string Target;
auto Err = utils::iterateAgentISAs(Agent, [&](hsa_isa_t ISA) {
@@ -3038,7 +3048,8 @@ struct AMDGPUPluginTy final : public GenericPluginTy {
if (Err)
return std::move(Err);
- if (!utils::isImageCompatibleWithEnv(Info, Target))
+ if (!utils::isImageCompatibleWithEnv(*ProcessorOrErr, *FlagsOrErr,
+ Target))
return false;
}
return true;
diff --git a/openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h b/openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
index 289dbf8e3d09d1..cac2432bfe3697 100644
--- a/openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
+++ b/openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h
@@ -13,6 +13,7 @@
#include <cstdint>
#include "Shared/Debug.h"
+#include "Utils/ELF.h"
#include "omptarget.h"
@@ -52,98 +53,64 @@ struct AMDGPUImplicitArgsTyCOV4 {
uint8_t Unused[56];
};
-uint32_t getImplicitArgsSize(uint16_t Version) {
+inline uint32_t getImplicitArgsSize(uint16_t Version) {
return Version < ELF::ELFABIVERSION_AMDGPU_HSA_V5
? sizeof(AMDGPUImplicitArgsTyCOV4)
: sizeof(AMDGPUImplicitArgsTy);
}
-/// Parse a TargetID to get processor arch and feature map.
-/// Returns processor subarch.
-/// Returns TargetID features in \p FeatureMap argument.
-/// If the \p TargetID contains feature+, FeatureMap it to true.
-/// If the \p TargetID contains feature-, FeatureMap it to false.
-/// If the \p TargetID does not contain a feature (default), do not map it.
-StringRef parseTargetID(StringRef TargetID, StringMap<bool> &FeatureMap) {
- if (TargetID.empty())
- return llvm::StringRef();
-
- auto ArchFeature = TargetID.split(":");
- auto Arch = ArchFeature.first;
- auto Features = ArchFeature.second;
- if (Features.empty())
- return Arch;
-
- if (Features.contains("sramecc+")) {
- FeatureMap.insert(std::pair<StringRef, bool>("sramecc", true));
- } else if (Features.contains("sramecc-")) {
- FeatureMap.insert(std::pair<StringRef, bool>("sramecc", false));
- }
- if (Features.contains("xnack+")) {
- FeatureMap.insert(std::pair<StringRef, bool>("xnack", true));
- } else if (Features.contains("xnack-")) {
- FeatureMap.insert(std::pair<StringRef, bool>("xnack", false));
- }
-
- return Arch;
-}
-
-/// Check if an image is compatible with current system's environment.
-bool isImageCompatibleWithEnv(const __tgt_image_info *Info,
- StringRef EnvTargetID) {
- llvm::StringRef ImageTargetID(Info->Arch);
-
- // Compatible in case of exact match.
- if (ImageTargetID == EnvTargetID) {
- DP("Compatible: Exact match \t[Image: %s]\t:\t[Env: %s]\n",
- ImageTargetID.data(), EnvTargetID.data());
- return true;
- }
-
- // Incompatible if Archs mismatch.
- StringMap<bool> ImgMap, EnvMap;
- StringRef ImgArch = utils::parseTargetID(ImageTargetID, ImgMap);
- StringRef EnvArch = utils::parseTargetID(EnvTargetID, EnvMap);
-
- // Both EnvArch and ImgArch can't be empty here.
- if (EnvArch.empty() || ImgArch.empty() || !ImgArch.contains(EnvArch)) {
- DP("Incompatible: Processor mismatch \t[Image: %s]\t:\t[Env: %s]\n",
- ImageTargetID.data(), EnvTargetID.data());
+/// Check if an image is compatible with current system's environment. The
+/// system environment is given as a 'target-id' which has the form:
+///
+/// <target-id> := <processor> ( ":" <target-feature> ( "+" | "-" ) )*
+///
+/// If a feature is not specific as '+' or '-' it is assumed to be in an 'any'
+/// and is compatible with either '+' or '-'. The HSA runtime returns this
+/// information using the target-id, while we use the ELF header to determine
+/// these features.
+inline bool isImageCompatibleWithEnv(StringRef ImageArch, uint32_t ImageFlags,
+ StringRef EnvTargetID) {
+ StringRef EnvArch = EnvTargetID.split(":").first;
+
+ // Trivial check if the base processors match.
+ if (EnvArch != ImageArch)
return false;
- }
- // Incompatible if image has more features than the environment,
- // irrespective of type or sign of features.
- if (ImgMap.size() > EnvMap.size()) {
- DP("Incompatible: Image has more features than the Environment \t[Image: "
- "%s]\t:\t[Env: %s]\n",
- ImageTargetID.data(), EnvTargetID.data());
- return false;
+ // Check if the image is requesting xnack on or off.
+ switch (ImageFlags & EF_AMDGPU_FEATURE_XNACK_V4) {
+ case EF_AMDGPU_FEATURE_XNACK_OFF_V4:
+ // The image is 'xnack-' so the environment cannot be 'xnack+'.
+ if (EnvTargetID.contains("xnack+"))
+ return false;
+ break;
+ case EF_AMDGPU_FEATURE_XNACK_ON_V4:
+ // The image is 'xnack+' so the environment cannot be 'xnack-'.
+ if (EnvTargetID.contains("xnack-"))
+ return false;
+ break;
+ case EF_AMDGPU_FEATURE_XNACK_UNSUPPORTED_V4:
+ case EF_AMDGPU_FEATURE_XNACK_ANY_V4:
+ default:
+ break;
}
- // Compatible if each target feature specified by the environment is
- // compatible with target feature of the image. The target feature is
- // compatible if the iamge does not specify it (meaning Any), or if it
- // specifies it with the same value (meaning On or Off).
- for (const auto &ImgFeature : ImgMap) {
- auto EnvFeature = EnvMap.find(ImgFeature.first());
- if (EnvFeature == EnvMap.end() ||
- (EnvFeature->first() == ImgFeature.first() &&
- EnvFeature->second != ImgFeature.second)) {
- DP("Incompatible: Value of Image's non-ANY feature is not matching with "
- "the Environment's non-ANY feature \t[Image: %s]\t:\t[Env: %s]\n",
- ImageTargetID.data(), EnvTargetID.data());
+ // Check if th eimage is requesting sramecc on or off.
+ switch (ImageFlags & EF_AMDGPU_FEATURE_SRAMECC_V4) {
+ case EF_AMDGPU_FEATURE_SRAMECC_OFF_V4:
+ // The image is 'sramecc-' so the environment cannot be 'sramecc+'.
+ if (EnvTargetID.contains("sramecc-"))
return false;
- }
+ break;
+ case EF_AMDGPU_FEATURE_SRAMECC_ON_V4:
+ // The image is 'sramecc+' so the environment cannot be 'sramecc-'.
+ if (EnvTargetID.contains("sramecc+"))
+ return false;
+ break;
+ case EF_AMDGPU_FEATURE_SRAMECC_UNSUPPORTED_V4:
+ case EF_AMDGPU_FEATURE_SRAMECC_ANY_V4:
+ break;
}
- // Image is compatible if all features of Environment are:
- // - either, present in the Image's features map with the same sign,
- // - or, the feature is missing from Image's features map i.e. it is
- // set to ANY
- DP("Compatible: Target IDs are compatible \t[Image: %s]\t:\t[Env: %s]\n",
- ImageTargetID.data(), EnvTargetID.data());
-
return true;
}
diff --git a/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h b/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
index ab6c457fba7864..103635f07b9b3f 100644
--- a/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
+++ b/openmp/libomptarget/plugins-nextgen/common/include/PluginInterface.h
@@ -1062,10 +1062,14 @@ struct GenericPluginTy {
return isValidDeviceId(SrcDeviceId) && isValidDeviceId(DstDeviceId);
}
+ /// Top level interface to verify if a given ELF image can be executed on a
+ /// given target. Returns true if the \p Image is compatible with the plugin.
+ Expected<bool> checkELFImage(__tgt_device_image &Image) const;
+
/// Indicate if an image is compatible with the plugin devices. Notice that
/// this function may be called before actually initializing the devices. So
/// we could not move this function into GenericDeviceTy.
- virtual Expected<bool> isImageCompatible(__tgt_image_info *Info) const = 0;
+ virtual Expected<bool> isELFCompatible(StringRef Image) const = 0;
/// Indicate whether the plugin supports empty images.
virtual bool supportsEmptyImages() const { return false; }
diff --git a/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.h b/openmp/libomptarget/plugins-nextgen/common/include/Utils/ELF.h
similarity index 75%
rename from openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.h
rename to openmp/libomptarget/plugins-nextgen/common/include/Utils/ELF.h
index 7b58cbaf59acef..068be1e46adba9 100644
--- a/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.h
+++ b/openmp/libomptarget/plugins-nextgen/common/include/Utils/ELF.h
@@ -21,9 +21,17 @@
namespace utils {
namespace elf {
-/// Return non-zero, if the given \p image is an ELF object, which
-/// e_machine matches \p target_id; return zero otherwise.
-int32_t checkMachine(__tgt_device_image *Image, uint16_t TargetId);
+/// Returns true or false if the \p Buffer is an ELF file.
+bool isELF(llvm::StringRef Buffer);
+
+/// Checks if the given \p Object is a valid ELF matching the e_machine value.
+llvm::Expected<bool> checkMachine(llvm::StringRef Object, uint16_t EMachine);
+
+/// Returns the processor string associated with the ELF flags.
+llvm::Expected<llvm::StringRef> getProcessor(llvm::StringRef Object);
+
+/// Get the associated flags from the ELF header.
+llvm::Expected<uint32_t> getFlags(llvm::StringRef Object);
/// Returns a pointer to the given \p Symbol inside of an ELF object.
llvm::Expected<const void *> getSymbolAddress(
diff --git a/openmp/libomptarget/plugins-nextgen/common/src/PluginInterface.cpp b/openmp/libomptarget/plugins-nextgen/common/src/PluginInterface.cpp
index 3c7d1ca8998787..3b07caca9d0def 100644
--- a/openmp/libomptarget/plugins-nextgen/common/src/PluginInterface.cpp
+++ b/openmp/libomptarget/plugins-nextgen/common/src/PluginInterface.cpp
@@ -1626,6 +1626,26 @@ Error GenericPluginTy::deinitDevice(int32_t DeviceId) {
return Plugin::success();
}
+Expected<bool> GenericPluginTy::checkELFImage(__tgt_device_image &Image) const {
+ StringRef Buffer(reinterpret_cast<const char *>(Image.ImageStart),
+ target::getPtrDiff(Image.ImageEnd, Image.ImageStart));
+
+ // First check if this image is a regular ELF file.
+ if (!utils::elf::isELF(Buffer))
+ return false;
+
+ // Check if this image is an ELF with a matching machine value.
+ auto MachineOrErr = utils::elf::checkMachine(Buffer, getMagicElfBits());
+ if (!MachineOrErr)
+ return MachineOrErr.takeError();
+
+ if (!*MachineOrErr)
+ return false;
+
+ // Perform plugin-dependent checks for the specific architecture if needed.
+ return isELFCompatible(Buffer);
+}
+
const bool llvm::omp::target::plugin::libomptargetSupportsRPC() {
#ifdef LIBOMPTARGET_RPC_SUPPORT
return true;
@@ -1653,44 +1673,26 @@ int32_t __tgt_rtl_init_plugin() {
}
int32_t __tgt_rtl_is_valid_binary(__tgt_device_image *TgtImage) {
+ // TODO: We should be able to perform a trivial ELF machine check without
+ // initializing the plugin first to save time if the plugin is not needed.
if (!Plugin::isActive())
return false;
- if (utils::elf::checkMachine(TgtImage, Plugin::get().getMagicElfBits()))
- return true;
-
- return Plugin::get().getJIT().checkBitcodeImage(*TgtImage);
-}
-
-int32_t __tgt_rtl_is_valid_binary_info(__tgt_device_image *TgtImage,
- __tgt_image_info *Info) {
- if (!Plugin::isActive())
- return false;
-
- if (!__tgt_rtl_is_valid_binary(TgtImage))
+ // Check if this is a valid ELF with a matching machine and processor.
+ auto MatchOrErr = Plugin::get().checkELFImage(*TgtImage);
+ if (Error Err = MatchOrErr.takeError()) {
+ [[maybe_unused]] std::string ErrStr = toString(std::move(Err));
+ DP("Failure to check validity of image %p: %s", TgtImage, ErrStr.c_str());
return false;
-
- // A subarchitecture was not specified. Assume it is compatible.
- if (!Info->Arch)
+ } else if (*MatchOrErr) {
return true;
-
- // Check the compatibility with all the available devices. Notice the
- // devices may not be initialized yet.
- auto CompatibleOrErr = Plugin::get().isImageCompatible(Info);
- if (!CompatibleOrErr) {
- // This error should not abort the execution, so we just inform the user
- // through the debug system.
- std::string ErrString = toString(CompatibleOrErr.takeError());
- DP("Failure to check whether image %p is valid: %s\n", TgtImage,
- ErrString.data());
- return false;
}
- bool Compatible = *CompatibleOrErr;
- DP("Image is %scompatible with current environment: %s\n",
- (Compatible) ? "" : "not", Info->Arch);
+ // Check if this is a valid LLVM-IR file with matching triple.
+ if (Plugin::get().getJIT().checkBitcodeImage(*TgtImage))
+ return true;
- return Compatible;
+ return false;
}
int32_t __tgt_rtl_supports_empty_images() {
diff --git a/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.cpp b/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.cpp
index 305ea7d9c874b4..75b4ba8f82cc76 100644
--- a/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.cpp
+++ b/openmp/libomptarget/plugins-nextgen/common/src/Utils/ELF.cpp
@@ -10,7 +10,7 @@
//
//===----------------------------------------------------------------------===//
-#include "ELF.h"
+#include "Utils/ELF.h"
#include "Shared/APITypes.h"
#include "Shared/Debug.h"
@@ -26,52 +26,228 @@ using namespace llvm;
using namespace llvm::ELF;
using namespace llvm::object;
-/// If the given range of bytes [\p BytesBegin, \p BytesEnd) represents
-/// a valid ELF, then invoke \p Callback on the ELFObjectFileBase
-/// created from this range, otherwise, return 0.
-/// If \p Callback is invoked, then return whatever value \p Callback returns.
-template <typename F>
-static int32_t withBytesAsElf(char *BytesBegin, char *BytesEnd, F Callback) {
- size_t Size = BytesEnd - BytesBegin;
- StringRef StrBuf(BytesBegin, Size);
-
- auto Magic = identify_magic(StrBuf);
- if (Magic != file_magic::elf && Magic != file_magic::elf_relocatable &&
- Magic != file_magic::elf_executable &&
- Magic != file_magic::elf_shared_object && Magic != file_magic::elf_core) {
- DP("Not an ELF image!\n");
- return 0;
+bool utils::elf::isELF(StringRef Buffer) {
+ switch (identify_magic(Buffer)) {
+ case file_magic::elf:
+ case file_magic::elf_relocatable:
+ case file_magic::elf_executable:
+ case file_magic::elf_shared_object:
+ case file_magic::elf_core:
+ return true;
+ default:
+ return false;
}
+}
- std::unique_ptr<MemoryBuffer> MemBuf =
- MemoryBuffer::getMemBuffer(StrBuf, "", false);
- Expected<std::unique_ptr<ObjectFile>> BinOrErr =
- ObjectFile::createELFObjectFile(MemBuf->getMemBufferRef(),
- /*InitContent=*/false);
- if (!BinOrErr) {
- DP("Unable to get ELF handle: %s!\n",
- toString(BinOrErr.takeError()).c_str());
- return 0;
+Expected<bool> utils::elf::checkMachine(StringRef Object, uint16_t EMachine) {
+ if (!isELF(Object))
+ return createError("Input is not an ELF.");
+
+ Expected<ELF64LEObjectFile> ElfOrErr =
+ ELF64LEObjectFile::create(MemoryBufferRef(Object, /*Identifier=*/""),
+ /*InitContent=*/false);
+ if (!ElfOrErr)
+ return ElfOrErr.takeError();
+
+ const auto Header = ElfOrErr->getELFFile().getHeader();
+ if (Header.e_ident[EI_CLASS] != ELFCLASS64)
+ return createError("Only 64-bit ELF files are supported");
+ if (Header.e_type != ET_EXEC && Header.e_type != ET_DYN)
+ return createError("Only executable ELF files are supported");
+
+ if (Header.e_machine == EM_AMDGPU) {
+ if (Header.e_ident[EI_OSABI] != ELFOSABI_AMDGPU_HSA)
+ return createError("Invalid AMD OS/ABI, must be AMDGPU_HSA");
+ if (Header.e_ident[EI_ABIVERSION] != ELFABIVERSION_AMDGPU_HSA_V4 &&
+ Header.e_ident[EI_ABIVERSION] != ELFABIVERSION_AMDGPU_HSA_V5)
+ return createError("Invalid AMD ABI version, must be version 4 or 5");
+ if ((Header.e_flags & EF_AMDGPU_MACH) < EF_AMDGPU_MACH_AMDGCN_GFX700 ||
+ (Header.e_flags & EF_AMDGPU_MACH) > EF_AMDGPU_MACH_AMDGCN_GFX1201)
+ return createError("Unsupported AMDGPU architecture");
+ } else if (Header.e_machine == EM_CUDA) {
+ if (~Header.e_flags & EF_CUDA_64BIT_ADDRESS)
+ return createError("Invalid CUDA addressing mode");
+ if ((Header.e_flags & EF_CUDA_SM) < EF_CUDA_SM...
[truncated]
|
jhuber6
force-pushed
the
ConvertToElf
branch
2 times, most recently
from
749c764 to
a63aa8a
Compare
|
Thanks for patch , Saiislam is actively reviewing over the next 2 days. |
jplehr
reviewed
Dec 18, 2023
| if (!Object) { | ||
| DP("Unknown ELF format!\n"); | ||
| return 0; | ||
| static Expected<StringRef> getAMDGPUProcessor(uint32_t Flags) { |
There was a problem hiding this comment.
Not really part of this patch, but should this generally live somewhere to be reused by more than just libomptarget?
There was a problem hiding this comment.
I could put it in ~/Documents/llvm/llvm-project/llvm/lib/BinaryFormat/ELF.cpp next to convertEMachineToArchName if needed.
There was a problem hiding this comment.
Can getAMDGPUCPUName from llvm/lib/Object/ELFObjectFile.cpp be used here instead of redefining this?
Otherwise, we will have to maintain this copy in sync with the original one at ELFObjectFile.cpp.
There was a problem hiding this comment.
Didn't realize that was there, but it's a private method. I can probably modify that to work here and add one for NVPTX instead.
|
This patch doesn't seem to work with HSA_XNACK=1 ./helloworld Both of them should work on a system which allows HSA_XNACK toggling on per process basis. I have couple of more concerns for which I am trying to get counter cases. |
So, the desired behavior here is that the prior should reject |
jhuber6
force-pushed
the
ConvertToElf
branch
from
a63aa8a to
94b8867
Compare
saiislam
reviewed
Dec 18, 2023
| "%s]\t:\t[Env: %s]\n", | ||
| ImageTargetID.data(), EnvTargetID.data()); | ||
| return false; | ||
| // Check if the image is requesting xnack on or off. |
There was a problem hiding this comment.
Why not use amdhsa.target string from the metadata and pass it is as is to the existing function?
https://llvm.org/docs/AMDGPUUsage.html#amdgpu-amdhsa-code-object-metadata-map-table-v4
There was a problem hiding this comment.
Does that net us anything? The information seems freely available in the ELF flags which is easier to retrieve than the AMDHSA metadata that's stored in a YAML form if memory serves. This way we don't need to parse anything and it's fast in the common case (No special flags set).
There was a problem hiding this comment.
If we get the information from the ELF, it makes sense to use it. Unsure why it's duplicated though.
doru1004
reviewed
Dec 18, 2023
The existing tests not breaking should be good enough. Testing the target-id stuff directly is difficult because it would depend on the platform you're running on. |
Could we add one of those CHECK tests which looks at the output for AMD GPUs perhaps and make sure it emits what we expect? Not sure if that is feasible, just a thought. |
jhuber6
force-pushed
the
ConvertToElf
branch
from
94b8867 to
2fba52a
Compare
Might be doable with unit tests in the future. We can just export more of this stuff as a single library. |
jdoerfert
approved these changes
Dec 18, 2023
| @@ -1651,44 +1671,26 @@ int32_t __tgt_rtl_init_plugin() { | |||
| } | |||
|
|
|||
| int32_t __tgt_rtl_is_valid_binary(__tgt_device_image *TgtImage) { | |||
| // TODO: We should be able to perform a trivial ELF machine check without | |||
| // initializing the plugin first to save time if the plugin is not needed. | |||
There was a problem hiding this comment.
Right, we need a "static" part and a post-init part. The former should weed out clear mismatches and the latter can check things like version numbers and flag support.
Follow up though.
| "%s]\t:\t[Env: %s]\n", | ||
| ImageTargetID.data(), EnvTargetID.data()); | ||
| return false; | ||
| // Check if the image is requesting xnack on or off. |
There was a problem hiding this comment.
If we get the information from the ELF, it makes sense to use it. Unsure why it's duplicated though.
saiislam
reviewed
Dec 19, 2023
| if (Header.e_ident[EI_ABIVERSION] != ELFABIVERSION_AMDGPU_HSA_V4 && | ||
| Header.e_ident[EI_ABIVERSION] != ELFABIVERSION_AMDGPU_HSA_V5) | ||
| return createError("Invalid AMD ABI version, must be version 4 or 5"); | ||
| if ((Header.e_flags & EF_AMDGPU_MACH) < EF_AMDGPU_MACH_AMDGCN_GFX700 || |
There was a problem hiding this comment.
Better to leave this check to the clang driver, backend, or hsa runtime. All archs between 700 and 1201 are not supported and it doesn't seem worth to check it again here.
There was a problem hiding this comment.
I mostly just wanted this as a sanity check for if someone ends up running something on a newer machine than we've accounted for. In the future the runtime is going to encompass more than what we support with OpenMP (like when I get around to writing a loader utilitly with the runtime like in libc ) So I wanted some basic checks beyond just the machine.
Summary: This patch reorganizes a lot of the code used to check for compatibility with the current environment. The main bulk of this patch involves moving from using a separate `__tgt_image_info` struct (which just contains a string for the architecture) to instead simply checking this information from the ELF directly. Checking information in the ELF is very inexpensive as creating an ELF file is simply writing a base pointer. The main desire to do this was to reorganize everything into the ELF image. We can then do the majority of these checks without first initializing the plugin. A future patch will move the first ELF checks to happen without initializing the plugin so we no longer need to initialize and plugins that don't have needed images. This patch also adds a lot more sanity checks for whether or not the ELF is actually compatible. Such as if the images have a valid ABI, 64-bit width, executable, etc. Fix Target-ID
jhuber6
force-pushed
the
ConvertToElf
branch
from
2fba52a to
aa38a47
Compare
|
Changed the patch a little, realized there was no need for the actual SM architecture name since we can just check the minor and major values with the flags directly. Got rid of the indirection around getting the platform flags and just use the ELF interface directly. |
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Dec 20, 2023
Reverts: buy time to land it [Libomptarget] Remove __tgt_image_info and use the ELF directly (llvm#75720) Change-Id: I5b8ced5747bca311d227478d62de58940d4bec47
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Dec 21, 2023
…#75720) Summary: This patch reorganizes a lot of the code used to check for compatibility with the current environment. The main bulk of this patch involves moving from using a separate `__tgt_image_info` struct (which just contains a string for the architecture) to instead simply checking this information from the ELF directly. Checking information in the ELF is very inexpensive as creating an ELF file is simply writing a base pointer. The main desire to do this was to reorganize everything into the ELF image. We can then do the majority of these checks without first initializing the plugin. A future patch will move the first ELF checks to happen without initializing the plugin so we no longer need to initialize and plugins that don't have needed images. This patch also adds a lot more sanity checks for whether or not the ELF is actually compatible. Such as if the images have a valid ABI, 64-bit width, executable, etc. Reverts following patches for reimplementation: 53859b6 b7e137c Change-Id: I37fc701d76d99464d7bd877168cbf912f622b634
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Dec 23, 2023
…ly (llvm#75720)" This reverts commit 027d1b1. Change-Id: I98e1fd370d4a3b2d793a075fd55f4c077b49acf5
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jan 12, 2024
…the ELF directly This patch is an adaptation of Joseph's upstream patch: llvm#75720 Change-Id: I60545c905d551282ec8833b140c715357a1d7378
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This patch reorganizes a lot of the code used to check for compatibility
with the current environment. The main bulk of this patch involves
moving from using a separate
__tgt_image_infostruct (which justcontains a string for the architecture) to instead simply checking this
information from the ELF directly. Checking information in the ELF is
very inexpensive as creating an ELF file is simply writing a base
pointer.
The main desire to do this was to reorganize everything into the ELF
image. We can then do the majority of these checks without first
initializing the plugin. A future patch will move the first ELF checks
to happen without initializing the plugin so we no longer need to
initialize and plugins that don't have needed images.
This patch also adds a lot more sanity checks for whether or not the ELF
is actually compatible. Such as if the images have a valid ABI, 64-bit
width, executable, etc.