[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:
> 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 .
I see. But I don't have an `StructType`. I would have to create it with only one field, that is, the array being allocated. I also found out that the offset calculated by `GetPointerBaseWithConstantOffset` always increases x8, 0, 8, 16 ..., no matter the type of the array. If I divide that offset by the size of the pointer type (given by `DataLayout`) then I get the index being accessed.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:840
+    V = getUnderlyingObject(SizesArg);
+    if (!isa<GlobalValue>(V)) {
+      if (!isa<AllocaInst>(V))
----------------
jdoerfert wrote:
> 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.
I think that's what it is doing, isn't it?. I only analyze `%offload_sizes` if it is not a global value. When it is a global value, is because it's a constant array, so we don't have to bother tracking the instructions for setting it up. Also, we have test cases for both. But I don't completely understand what should I do.


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

https://reviews.llvm.org/D86300