[Openmp-commits] [PATCH] D97680: [OpenMP] Simplify GPU memory globalization

[Shilei Tian via Phabricator via Openmp-commits]

tianshilei1992 added a comment.

Generally LGTM. Some nits.



================
Comment at: clang/include/clang/Driver/Options.td:2331
   PosFlag<SetTrue, [CC1Option]>, NegFlag<SetFalse>, BothFlags<[NoArgumentUnused, HelpHidden]>>;
-def fopenmp_cuda_parallel_target_regions : Flag<["-"], "fopenmp-cuda-parallel-target-regions">, Group<f_Group>,
-  Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,
----------------
Can we make these removal of options in another patch if it doesn't affect remaining functionality?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1731
         llvm::ConstantInt::get(CGF.SizeTy, Align.getQuantity());
+
     Size = Bld.CreateUDiv(Size, AlignVal);
----------------
unrelated


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:3885
     return;
+
   CheckVarsEscapingDeclContext VarChecker(CGF, TeamAndReductions.second);
----------------
unrelated change


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:3907
     assert(VD->isCanonicalDecl() && "Expected canonical declaration");
-    const FieldDecl *FD = VarChecker.getFieldForGlobalizedVar(VD);
-    Data.insert(std::make_pair(VD, MappedVarData(FD, IsInTTDRegion)));
+    Data.insert(std::make_pair(VD, MappedVarData()));
   }
----------------
Not sure that's the reason you need the explicit constructor. If yes, can we get around it somehow?


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.h:442
   struct MappedVarData {
+    MappedVarData() = default;
     /// Corresponding field in the global record.
----------------
Constructor is not needed here I suppose?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97680