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

[Hamilton Tobon Mosquera via Phabricator via llvm-commits]

hamax97 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.
----------------
jdoerfert wrote:
> 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.
That function is pretty handy, thank you. What I don't find is the helper for returning the index given the offset.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:775
 
+      OffloadArrayPtr OffloadArrays[3];
+      if (!getValuesInOffloadArrays(*RTCall, OffloadArrays))
----------------
jdoerfert wrote:
> 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.
What do you mean by make `initialize` a member function?. A member of `OpenMPOpt`?.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:840
+    V = getUnderlyingObject(SizesArg);
+    if (!isa<GlobalValue>(V)) {
+      if (!isa<AllocaInst>(V))
----------------
jdoerfert wrote:
> Why the global check here?
The `%offload_sizes` is sometimes a global array. If it is, the idea is to not analyze it.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:885
+      LLVM_DEBUG(dbgs() << "\t\toffload_sizes: " << Printer.str() << "\n");
+    }
+  }
----------------
jdoerfert wrote:
> 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.
Hahaha. I'm fine with both ways.


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

https://reviews.llvm.org/D86300