JonChesterfield added inline comments.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1344
+FieldDecl *clang::CodeGen::CodeGenUtil::CodeGenUtil::addFieldToRecordDecl(
+ ASTContext &C, DeclContext *DC, QualType FieldTy) {
----------------
This appears to be the same as the free function we had before, except now all
the call sites are prefixed CodegenUtil. Is there a functional change I'm
missing?
The rest of this patch would be easier to read with this part split off.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:77
+
+void CGOpenMPRuntimeAMDGCN::emitNonSPMDKernelWrapper(
+ const OMPExecutableDirective &D, StringRef ParentName,
----------------
This is a very verbose way to say that amdgcn calls emitmetatdata at the end of
emitkernel and nvptx doesn't. Suggest unconditionally calling emitmetatdata,
and having emitmetatdata be a no-op for nvptx.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:89
+//
+void CGOpenMPRuntimeAMDGCN::setPropertyWorkGroupSize(CodeGenModule &CGM,
+ StringRef Name,
----------------
I think there's a credible chance this is useful to nvptx, so doesn't have to
be amdgcn specific
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:108
+ bool FlatAttrEmitted = false;
+ unsigned DefaultWorkGroupSz =
+ CGM.getTarget().getGridValue(llvm::omp::GVIDX::GV_Default_WG_Size);
----------------
I think this is about computing a maximum workgroup size which the runtime uses
to limit the number of threads it launches. If so, this is probably useful for
nvptx and amdgcn. I'm having trouble working out what the conditions are
though. Maybe it's based on an openmp clause?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:150
+ // emit amdgpu-flat-work-group-size if not emitted already.
+ if (!FlatAttrEmitted) {
+ std::string FlatAttrVal = llvm::utostr(DefaultWorkGroupSz);
----------------
I think I remember seeing a diff that makes this attribute unconditionally
emitted by some other part of the toolchain. If so, it may no longer be required
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:169
+ // Create all device images
+ llvm::Constant *AttrData[] = {
+ llvm::ConstantInt::get(CGM.Int16Ty, 2), // Version
----------------
HostServices is unused. Mode is redundant with exec_mode. wg_size is redundant
with the other wg_size symbol added above. This kern_desc object should be
deleted, not upstreamed.
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
