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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:150
+    private:
+      bool getLastAccessesToOfflArray(Instruction *Before = nullptr);
+
----------------
hamax97 wrote:
> 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.
I see. Could we make it less generic and more specific for now. This is by nature a complex thing and I would prefer the first version to be rather targeted.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:292
+    }
+  }
+  return true;
----------------
hamax97 wrote:
> 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?
I would suggest to *only* handle what clang generates right now. And to test for it specifically. No extra uses, no weird types, nothing.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:298
+  const uint64_t NumValues =
+      Array->getAllocatedType()->getArrayNumElements();
+  initialize(NumValues);
----------------
hamax97 wrote:
> 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.
There are two levels of "array" here. AllocaInst can allocate an array of the given type and the given type can be an array. check for what you expect (I think the latter).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:312
+    return false;
+  }
+
----------------
hamax97 wrote:
> 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.
That was just a suggestion. Any (simple) way to identify what we need to is fine with me.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:314
+
+  Array->reverseUseList(); // To traverse users from top to bottom.
+  for (auto *Usr : Array->users()) {
----------------
hamax97 wrote:
> 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.
Oh, I see. That is where my basic block traversal comes in. If you want to know if it is "before `Before`", actually verify that, e.g., by traversing the basic block from `Before` to the beginning. All the instructions you see are "before `Before`. The use list is not order in any specific way.


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

https://reviews.llvm.org/D82719