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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:517
+  int64_t getAccessedIdx(StoreInst &S) {
+    auto *Dst = S.getOperand(1);
+    // Unrecognized store pattern.
----------------
In line 499 and 500 you compute the Dst already and compare it to Array and here again. Maybe use `GetPointerBaseWithConstantOffset` instead (above) and pass in the offset. That way you don't traverse the use chain twice with different "methods" and there is no special case for the location 0. You take the array element type and ask for the index to the offset, that should be available via a helper in the struct type, IIRC.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:775
 
+      OffloadArrayPtr OffloadArrays[3];
+      if (!getValuesInOffloadArrays(*RTCall, OffloadArrays))
----------------
Let's keep this simple for now. Just an array of 3 `OffloadArray` objects. Make the initilize a member function and no need for the extra pointers and stuff.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:840
+    V = getUnderlyingObject(SizesArg);
+    if (!isa<GlobalValue>(V)) {
+      if (!isa<AllocaInst>(V))
----------------
Why the global check here?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:885
+      LLVM_DEBUG(dbgs() << "\t\toffload_sizes: " << Printer.str() << "\n");
+    }
+  }
----------------
I somehow feel it would be better to print them by index, so that all information for one item is together, though it's not a strong feeling.


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

https://reviews.llvm.org/D86300