[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/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