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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.
Herald added a subscriber: danielkiss.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:517
+  int64_t getAccessedIdx(StoreInst &S) {
+    auto *Dst = S.getOperand(1);
+    // Unrecognized store pattern.
----------------
hamax97 wrote:
> 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.
I forgot half of it:

Take the datalayout, use it with the struct type to get a StructLayout (`const StructLayout *getStructLayout(StructType *Ty) const;`).
The struct layout has a method: getElementContainingOffset that does what u want, though you need to verify you are at the beginning of the element with getElementOffset .


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:775
 
+      OffloadArrayPtr OffloadArrays[3];
+      if (!getValuesInOffloadArrays(*RTCall, OffloadArrays))
----------------
hamax97 wrote:
> 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`?.
remove the static keyword basically


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:840
+    V = getUnderlyingObject(SizesArg);
+    if (!isa<GlobalValue>(V)) {
+      if (!isa<AllocaInst>(V))
----------------
hamax97 wrote:
> 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.
be convservative now: pick one use case and go with that, a negative test for the other with a fixme is good too.


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

https://reviews.llvm.org/D86300