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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some style and minor remarks together with some proposals.



================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:150
+    private:
+      bool getLastAccessesToOfflArray(Instruction *Before = nullptr);
+
----------------
Do we expect to call this with a nullptr? If not, we should make it a reference instead.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:152
+
+      bool getLastStoresInOfflArray();
+
----------------
Nit: I guess adding the missing characters of `Offload` will only help ;). 

StoresTo or do you mean stores into pointers that are in the offload array?


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:175
+    bool getValuesInOfflArrays();
+  };
+
----------------
We need documentation for all the members and methods. The comment on the struct should also be expanded, e.g., list all the thing we store in here.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:247
+  /// Splits a runtime call that involves a host to device transfer into its ""
+  bool splitMemoryTransfer(MemoryTransfer &MT);
+
----------------
Comment broken?


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:269
+  /// device memory transfers.
+  bool hideMemTransfersLatency();
+
----------------
Feel free to elaborate here a bit what is going to happen. Or in the file comment.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:231
 
-  auto RTFTyIt = RTFArgTypes.begin();
+  auto *RTFTyIt = RTFArgTypes.begin();
   for (Argument &Arg : F->args()) {
----------------
Just commit this change.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:249
+  const unsigned PtrsArgNum = 3; // **offload_ptrs.
+  const unsigned SizesArgNum = 4; // **offload_sizes.
+  auto *BasePtrsArg = RuntimeCall->arg_begin() + BasePtrsArgNum;
----------------
Style: Maybe put a comment in the beginning explaining how the function declaration looks. I also prefer comments in their own lines, generally as full sentences.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:253
+  auto *SizesArg = RuntimeCall->arg_begin() + SizesArgNum;
+  const auto &DL = InfoCache.getDL();
+
----------------
Nit: Write the types here, usually helps.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:292
+    }
+  }
+  return true;
----------------
Are we OK if any of these is not an AllocaInst? Or should we also bail in that situations? Do we have a test for the problematic cases?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:298
+  const uint64_t NumValues =
+      Array->getAllocatedType()->getArrayNumElements();
+  initialize(NumValues);
----------------
You need to verify before that the type is an array type.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:302
+  return getLastAccessesToOfflArray(Before) && getLastStoresInOfflArray();
+}
+
----------------
Maybe merge initialize in this method and call this method initialize. Or, take the runtime call do it all in the constructor. Afterwards you can ask if the object is "valid".


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:312
+    return false;
+  }
+
----------------
I guess for now we could assume the stores are in the same block as Before? We could iterate from Before to the basic block begin, WDYT?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:314
+
+  Array->reverseUseList(); // To traverse users from top to bottom.
+  for (auto *Usr : Array->users()) {
----------------
We should never rely on the order of use lists or other things. We should also never modify the order ;)

Assume users are in any order, there is no guarantee otherwise anyway.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:317
+    if (!isa<Instruction>(Usr))
+      continue;
+
----------------
Unexpected users should be reason to give up.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:729
+    Changed = splitMemoryTransfer(MT);
+    return Changed;
+  };
----------------
You clobber the outer `Changed` variable here. So even if it was true at some point, you might return false. You should also be careful because the `foreachUse` callback expects you to return `true` only if the use was deleted.


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

https://reviews.llvm.org/D82719