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

[Hamilton Tobon-Mosquera via Phabricator via llvm-commits]

hamax97 marked 2 inline comments as done.
hamax97 added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:285
+/// host to device memory offloading.
+struct HideMemTransferLatency {
+
----------------
jdoerfert wrote:
> sstefan1 wrote:
> > 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.
> I'm not convinced that we need to expose an optimization that runs as part of OpenMPOpt (only) to the world.
> 
> Why do we need all this code in the (public) header? If you really want to separate it, I guess a private header in the lib folder would suffice, right? Or do you expect other code to interact with this?
Well, the idea is to make this easily testable. So, if having a private header, we will need to include it in the unittests. If that is not a problem, then it is a better idea having the private header. What do you think then?


================
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.
----------------
sstefan1 wrote:
> Not sure we need these 'end of' comments.
Sure, I'll remove them.


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

https://reviews.llvm.org/D82719