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

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam marked 3 inline comments as done.
saiislam added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:175
+
+void CGOpenMPRuntimeAMDGCN::emitSPMDKernelWrapper(
+    const OMPExecutableDirective &D, StringRef ParentName,
----------------
JonChesterfield wrote:
> 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?
It will be then difficult to track what all things are being done differently in the two. So, the common code has been generalized and (no change in nvptx + some changes in amdgcn) has been used as specialization.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h:49
+
+  /// Allocate global variable for TransferMedium
+  llvm::GlobalVariable *
----------------
JonChesterfield wrote:
> 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.
> 
> 
I understand what you are suggesting. But, there are multiple such variables where linkage between nvptx and amdgcn are different. Also current style gives flexibility to a future implementation to define these variables in their own way.
What do you think?


================
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.
----------------
JonChesterfield wrote:
> Please put this back to the previous location so we can see whether it changed in the diff
This movement changes them from private to protected.
I could have just added access specifiers and not move the definitions. It would have simplified the review, but it would have decreased the readability for future.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:217-249
+  /// Allocate global variable for TransferMedium
+  virtual llvm::GlobalVariable *
+  allocateTransferMediumGlobal(CodeGenModule &CGM, llvm::ArrayType *Ty,
+                               StringRef TransferMediumName) = 0;
+
+  /// Allocate global variable for SharedStaticRD
+  virtual llvm::GlobalVariable *
----------------
ABataev wrote:
> Are all these required to be public?
Yes, they are being called from outside class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86097