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

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

hamax97 marked 4 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 {
+
----------------
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.


================
Comment at: llvm/unittests/Transforms/IPO/OpenMPOpt/HideMemTransferLatency.cpp:13
+
+class HideMemTransferLatencyTest : public OpenMPOptTest {
+protected:
----------------
Here are the unittests for this specific optimization.


================
Comment at: llvm/unittests/Transforms/IPO/OpenMPOpt/HideMemTransferLatency.cpp:23
+    auto &RFI = OMPInfoCache->RFIs[OMPRTL___tgt_target_data_begin];
+    auto GetCallSites = [&](Use &U, Function &Decl) {
+      auto *RTCall = OpenMPOpt::getCallIfRegularCall(U, &RFI);
----------------
I don't know if this is a good approach. The uses of the runtime call are first stored in the MemTransferCalls, and later used in the test fixture. I could've included this in the test itself, but that would mean writing it over and over again.


================
Comment at: llvm/unittests/Transforms/IPO/OpenMPOpt/OpenMPOptTest.h:21
+
+class OpenMPOptTest : public ::testing::Test {
+protected:
----------------
This can be the base class for testing each separate optimization. The CallGraphSCC can be moved as a class attribute, it's just that I don't use it for now.


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

https://reviews.llvm.org/D82719