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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, I added a bunch of minor change requests, nothing major.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:450
+  /// Physical array (in the IR).
+  AllocaInst *Array;
+  /// Mapped values.
----------------
initialize to nullptr


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:467
+    this->Array = &Array;
+    if (!getValues(Before))
+      return false;
----------------
Nit: pass Array to getValues, and initialize the member only if that was successful.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:505
+      if (Dst == Array) {
+        int64_t Idx = Offset / PointerSize;
+        StoredValues[Idx] = getUnderlyingObject(S->getValueOperand());
----------------
bail if `Idx * PointerSize != Offset`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:742
+#ifndef NDEBUG
+      debugValuesInOffloadArrays(OffloadArrays);
+#endif
----------------
Make this:
`LLVM_DEBUG(...)`

the test then requires the -debug-only=openmpopt flag


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:802
+    if (isa<GlobalValue>(V))
+      return true;
+    if (!isa<AllocaInst>(V))
----------------
return true only if the global is a constant, false otherwise. so "`return global->isconstant`"


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:817
+  /// TODO: Move this to a unittest when unittests are available for OpenMPOpt.
+  void debugValuesInOffloadArrays(ArrayRef<OffloadArray> OAs) {
+    assert(OAs.size() == 3 && "There are three offload arrays to debug!");
----------------
call this "printXXX" or "dumpXXX"


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

https://reviews.llvm.org/D86300