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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:684
         const OffloadDeviceGlobalVarEntryInfoActTy &Action);
-
   private:
----------------
Restore original formatting


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:2482-2487
+FieldDecl *addFieldToRecordDecl(ASTContext &C, DeclContext *DC,
+                                QualType FieldTy);
+
+template <class... As>
+llvm::GlobalVariable *createGlobalStruct(CodeGenModule &CGM, QualType Ty,
+                                         bool IsConstant,
----------------
Better to encapsulate these functions into a new utility class and make them public static.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.h:498
   /// far.
+
   class OffloadEntriesInfoManagerTy {
----------------
ABataev wrote:
> Remove unnecessary formatting changes. 
Still not removed


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h:30-63
+  /// Curret nesting level of parallel region
+  int ParallelLevel = 0;
+
+  /// Maximum nesting level of parallel region
+  int MaxParallelLevel = 0;
+
+  /// Struct to store kernel descriptors
----------------
Do you really need to expose all these new members as public?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h:31
+  /// Curret nesting level of parallel region
+  int ParallelLevel = 0;
+
----------------
Runtime does not support nested parallelism on GPU. Do you really need it?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.h:98
+
+  /// AMDGCN specific PrePostActionTy implementation
+  class AMDGCNPrePostActionTy final : public PrePostActionTy {
----------------
It does not help to understand the functionality


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1957
+  auto &RT = static_cast<CGOpenMPRuntimeGPU &>(CGM.getOpenMPRuntime());
+  PrePostActionTy *Action = RT.getPrePostActionTy();
+  CodeGen.setAction(*Action);
----------------
It leads to a mem leak.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:37
+  /// Linkage type of StaticRD Global variable
+  llvm::GlobalValue::LinkageTypes StaticRDLinkage;
+
----------------
1. Make it private or protected
2.Add default initializer


================
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 *
----------------
Are all these required to be public?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:415-417
+  /// true if we're definitely in the parallel region.
+  bool IsInParallelRegion = false;
+
----------------
Make it private or protected


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86097