[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:175
+
+void CGOpenMPRuntimeAMDGCN::emitSPMDKernelWrapper(
+    const OMPExecutableDirective &D, StringRef ParentName,
----------------
The nvptx emitSPMDKernelWrapper does nothing and the amdgcn one appends some metadata.

How about 'nvptx::generateMetadata(...)' that does nothing and 'amdgcn::generateMetadata(...)` that does this stuff, called from the end of emitSPMDKernel?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:197
+
+/// Emit structure descriptor for a kernel
+void CGOpenMPRuntimeAMDGCN::emitStructureKernelDesc(
----------------
This metadata generation could be split out from the other changes.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h:49
+
+  /// Allocate global variable for TransferMedium
+  llvm::GlobalVariable *
----------------
I'm not convinced by this abstraction. It looks like amdgcn and nvptx want almost exactly the same variable in each case. The difference appears to be that nvptx uses internal linkage and amdgcn uses weak + externally initialized, in which case we're better off with

`bool nvptx::needsExternalInitialization() {return false;}`
`bool amdgpu::needsExternalInitialization() {return true;}`

Or, if the inline ternary is unappealing, amdgcn::NewGlobalVariable(...) that passes the arguments to llvm::GlobalVariable while setting the two fields that differ between the two.




================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:175
 
+  /// Emit outlined function specialized for the Fork-Join
+  /// programming model for applicable target directives on the NVPTX device.
----------------
Please put this back to the previous location so we can see whether it changed in the diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86097