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

[Hamilton Tobon-Mosquera via Phabricator via llvm-commits]

hamax97 marked 6 inline comments as done.
hamax97 added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:150
+    private:
+      bool getLastAccessesToOfflArray(Instruction *Before = nullptr);
+
----------------
jdoerfert wrote:
> Do we expect to call this with a nullptr? If not, we should make it a reference instead.
The idea is to make that lower bound in the function optional. So, if it's not given, then the values returned would be the last values stored. This, assuming that there might be multiple stores to the offload array.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:292
+    }
+  }
+  return true;
----------------
jdoerfert wrote:
> 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?
We can handle ArrayTypes too, I'll fix that.
We don't have a test for non alloca arrays?. Do we need it if we handle ArrayTypes too?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:298
+  const uint64_t NumValues =
+      Array->getAllocatedType()->getArrayNumElements();
+  initialize(NumValues);
----------------
jdoerfert wrote:
> You need to verify before that the type is an array type.
The problem with `isArrayAllocation()` is that it returns false if the array has size 1.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:302
+  return getLastAccessesToOfflArray(Before) && getLastStoresInOfflArray();
+}
+
----------------
jdoerfert wrote:
> 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".
Sure. That sounds nicer.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:312
+    return false;
+  }
+
----------------
jdoerfert wrote:
> 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?
Yes we can assume they are in the same BasicBlock. But why would we want to iterate from `Before`'s BasicBlock to the `entry` BasicBlock?, I don't fully understand your point.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:314
+
+  Array->reverseUseList(); // To traverse users from top to bottom.
+  for (auto *Usr : Array->users()) {
----------------
jdoerfert wrote:
> 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.
Mmmmm. The problem would be knowing when to stop. The idea here is to go from top to bottom and stop if we go over `Before`. But if there's no sense of direction how would you know how to stop?. I thought using MemorySSA, we can traverse upwards the definition chain starting in the call site.


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

https://reviews.llvm.org/D82719