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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
