[Openmp-commits] [PATCH] D139730: [OpenMP][DeviceRTL][AMDGPU] Support code object version 5

[Matt Arsenault via Phabricator via Openmp-commits]

arsenm added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:17057
+  Constant *Offset, *OffsetOld;
+  Value *DP, *DP1;
+
----------------
Spell out to DispatchPtr?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1206-1208
+  getTargetCodeGenInfo().emitTargetGlobals(*this);
+
   getTargetCodeGenInfo().emitTargetMetadata(*this, MangledDeclNames);
----------------
These could be one combined hook? this isn't really different from metadata


================
Comment at: clang/lib/CodeGen/Targets/AMDGPU.cpp:369-386
+    if (CGM.getModule().getNamedGlobal(Name))
+      return;
+
+    auto *Type =
+        llvm::IntegerType::getIntNTy(CGM.getModule().getContext(), Size);
+    auto *GV = new llvm::GlobalVariable(
+        CGM.getModule(), Type, true, Linkage,
----------------
You moved GetOrCreateLLVMGlobal but don't use it? 

The lamdba is unnecessary for a single local use


================
Comment at: clang/lib/Driver/ToolChain.cpp:1369
     if (A->getOption().matches(options::OPT_m_Group)) {
-      if (SameTripleAsHost)
+      // pass code objection version to device toolchain
+      // to correctly set meta-data in intermediate files
----------------
Capitalize


================
Comment at: clang/lib/Driver/ToolChain.cpp:1372
+      if (SameTripleAsHost ||
+          A->getOption().matches(options::OPT_mcode_object_version_EQ))
         DAL->append(A);
----------------
Don't understand why this is necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139730