[Openmp-commits] [PATCH] D139730: [OpenMP][DeviceRTL][AMDGPU] Support code object version 5

[Joseph Huber via Phabricator via Openmp-commits]

jhuber6 added a comment.

Some nits. I'm assuming we're getting the code object in the backend now? We'll need to make sure that `-Wl,--amdhsa-code-object-version` is passed to the clang invocation inside of the `clang-linker-wrapper` to handle `-save-temps` mode.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:17053
+/// Note: "llvm.amdgcn.abi.version" is supposed to be emitted and intialized by
+///       the clang during compilation of user code.
 Value *EmitAMDGPUWorkGroupSize(CodeGenFunction &CGF, unsigned Index) {
----------------



================
Comment at: clang/lib/Driver/ToolChain.cpp:1363
   for (auto *A : Args) {
+
     // Exclude flags which may only apply to the host toolchain.
----------------
Random whitespace.


================
Comment at: clang/test/CodeGenCUDA/amdgpu-code-object-version-linking.cu:54
+#endif
\ No newline at end of file

----------------
Need newline


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/src/rtl.cpp:3016
+  if (getImplicitArgsSize() < utils::COV5_SIZE) {
+    DP("Setting fields of ImplicitArgs for COV4\n");
+  } else {
----------------
Don't think this needs to be a debug message, same below


================
Comment at: openmp/libomptarget/plugins-nextgen/amdgpu/utils/UtilitiesRTL.h:36
 
-// The implicit arguments of AMDGPU kernels.
-struct AMDGPUImplicitArgsTy {
-  uint64_t OffsetX;
-  uint64_t OffsetY;
-  uint64_t OffsetZ;
-  uint64_t HostcallPtr;
-  uint64_t Unused0;
-  uint64_t Unused1;
-  uint64_t Unused2;
+enum IMPLICITARGS : uint32_t {
+  COV4_SIZE = 56,
----------------
I'm still not a fan of replacing the struct. The mnemonic of having a struct is much more user friendly.
```
ImplicitArgsTy Args{};
std::memset(&Args, sizeof(ImplicitArgsTy), 0);
...
```
If we don't use something, just make it some random bytes, e.g.
```
struct ImplicitArgsTy {
  uint64_t OffsetX;
  uint8_t Unused[64]; // 64 byte offset.
};
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139730/new/

https://reviews.llvm.org/D139730