[PATCH] D82719: [OpenMPOpt][SplitMemTransfer][WIP] Getting values stored in offload arrays

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 added a comment.

I think you should split this in 2 patches. One being the refactoring. (this happened to me with the ICV patch)

But lets hear from Johannes as well.



================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:285
+/// host to device memory offloading.
+struct HideMemTransferLatency {
+
----------------
hamax97 wrote:
> IMHO it would nice to have the optimizations outside OpenMPOpt. It's easier testing them like this, and a good practice according to Googletest, [[ https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#testing-private-code | Testing private code ]].
> It's also easier adding new features to them.
I'm ok with having this here. I guess it would be good to add some documentation comments in the .h files.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:179
 
-struct OpenMPOpt {
-
-  using OptimizationRemarkGetter =
-      function_ref<OptimizationRemarkEmitter &(Function *)>;
-
-  OpenMPOpt(SmallVectorImpl<Function *> &SCC, CallGraphUpdater &CGUpdater,
-            OptimizationRemarkGetter OREGetter,
-            OMPInformationCache &OMPInfoCache)
-      : M(*(*SCC.begin())->getParent()), SCC(SCC), CGUpdater(CGUpdater),
-        OREGetter(OREGetter), OMPInfoCache(OMPInfoCache) {}
-
-  /// Run all OpenMP optimizations on the underlying SCC/ModuleSlice.
-  bool run() {
-    bool Changed = false;
-
-    LLVM_DEBUG(dbgs() << TAG << "Run on SCC with " << SCC.size()
-                      << " functions in a slice with "
-                      << OMPInfoCache.ModuleSlice.size() << " functions\n");
-
-    /// Print initial ICV values for testing.
-    /// FIXME: This should be done from the Attributor once it is added.
-    if (PrintICVValues) {
-      InternalControlVar ICVs[] = {ICV_nthreads, ICV_active_levels, ICV_cancel};
-
-      for (Function *F : OMPInfoCache.ModuleSlice) {
-        for (auto ICV : ICVs) {
-          auto ICVInfo = OMPInfoCache.ICVs[ICV];
-          auto Remark = [&](OptimizationRemark OR) {
-            return OR << "OpenMP ICV " << ore::NV("OpenMPICV", ICVInfo.Name)
-                      << " Value: "
-                      << (ICVInfo.InitValue
-                              ? ICVInfo.InitValue->getValue().toString(10, true)
-                              : "IMPLEMENTATION_DEFINED");
-          };
-
-          emitRemarkOnFunction(F, "OpenMPICVTracker", Remark);
-        }
+//===----------------------------------------------------------------------===//
+// End of definitions of the OMPInformationCache helper structure.
----------------
Not sure we need these 'end of' comments.


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

https://reviews.llvm.org/D82719