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