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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Can you extract the NFC code movement changes. It is impossible to determine what is new here and what is just moved/rewritten.



================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:285
+/// host to device memory offloading.
+struct HideMemTransferLatency {
+
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:12
 // - Deduplication of runtime calls, e.g., omp_get_thread_num.
+// - Latency hiding of host to device memory transfers.
 //
----------------
Yay! :)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:181
+// End of definitions of the OMPInformationCache helper structure.
+//===----------------------------------------------------------------------===//
+
----------------
I don't think they help


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

https://reviews.llvm.org/D82719