Page MenuHomePhabricator

[OpenMPOpt][SplitMemTransfer] Getting values stored in offload arrays
ClosedPublic

Authored by hamax97 on Aug 20 2020, 9:02 AM.

Details

Summary

getValuesInOffloadArrays goes through the offload arrays in __tgt_target_data_begin_mapper getting the values stored in them before the call is issued.

call void @__tgt_target_data_begin_mapper(arg0, arg1,
    i8** %offload_baseptrs, i8** %offload_ptrs, i64* %offload_sizes,
...)

PD: I tried hard to keep it small.

Diff Detail

Event Timeline

hamax97 created this revision.Aug 20 2020, 9:02 AM
hamax97 requested review of this revision.Aug 20 2020, 9:02 AM
jdoerfert added inline comments.Aug 20 2020, 9:52 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
761

Don't you need a reference here? We also need a test, maybe just one that prints the result?

hamax97 added inline comments.Aug 20 2020, 10:04 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
761

If I pass an array it's passed as a pointer, isn't it?.

A unittest?

jdoerfert added inline comments.Aug 20 2020, 10:49 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
761

I see. Make it an ArrayRef<OffloadArrayPtr> instead. That will make it clear and you should not need to modify the call site.

A unit test or a lit test hat looks for some output that we print for debug purposes.

hamax97 updated this revision to Diff 286920.Aug 20 2020, 4:56 PM
  • Adding regression test.
sstefan1 added inline comments.Aug 21 2020, 2:56 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
488

I think it is preferred to have this on the new line. There's at least one more below.

Also I think it is preferred to use the actual type in the loop above.

hamax97 updated this revision to Diff 287045.Aug 21 2020, 8:48 AM

NIT: Refactor some variable declarations.

jdoerfert added inline comments.Aug 21 2020, 9:38 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
451

Style: Avoid comments in the same line as code (always, except in calls like ,/* .. */ foo) ). Put them before the declaration please.

llvm/test/Transforms/OpenMP/values_in_offload_arrays.ll
4

you need to require assertions (IIRC) otherwise this tests fails (as non-assert builds do not have debug print)

hamax97 updated this revision to Diff 287058.Aug 21 2020, 9:59 AM
  • Require comments
  • Added REQUIRES: assertions to regression test.
hamax97 updated this revision to Diff 287702.Aug 25 2020, 10:17 AM
  • NIT changes.
  • Tried to split the patch, but then we couldn't test for the values we got.
jdoerfert added inline comments.Aug 27 2020, 11:10 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
517

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.

737

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.

802

Why the global check here?

847

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.

hamax97 added inline comments.Aug 28 2020, 9:25 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
517

That function is pretty handy, thank you. What I don't find is the helper for returning the index given the offset.

737

What do you mean by make initialize a member function?. A member of OpenMPOpt?.

802

The %offload_sizes is sometimes a global array. If it is, the idea is to not analyze it.

847

Hahaha. I'm fine with both ways.

jdoerfert added inline comments.Aug 28 2020, 12:58 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
517

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 .

737

remove the static keyword basically

802

be convservative now: pick one use case and go with that, a negative test for the other with a fixme is good too.

hamax97 added inline comments.Aug 28 2020, 1:22 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
517

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.

802

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.

jdoerfert added inline comments.Aug 28 2020, 2:49 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
517

oh, it's an array, right. you can use the store size of the element type and divide the offset by it, it should be a multiple and that gives you the index.

802

I guess this is OK. maybe use an early exist though:

if (global)
  return true.

you also need to consider that OAs[2] is later maybe unitialized, but that is for follow up patches.

hamax97 updated this revision to Diff 288964.Aug 31 2020, 9:22 AM
  • Removing factory function for OffloadArray, now using it as member function.
  • Calculating the accessed index of the OffloadArray using the offset returned by GetPointerBaseWithConstantOffset.
jdoerfert accepted this revision.Aug 31 2020, 9:43 AM

LG, I added a bunch of minor change requests, nothing major.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
450

initialize to nullptr

467

Nit: pass Array to getValues, and initialize the member only if that was successful.

505

bail if Idx * PointerSize != Offset.

742

Make this:
LLVM_DEBUG(...)

the test then requires the -debug-only=openmpopt flag

802

return true only if the global is a constant, false otherwise. so "return global->isconstant"

817

call this "printXXX" or "dumpXXX"

This revision is now accepted and ready to land.Aug 31 2020, 9:43 AM
hamax97 added inline comments.Aug 31 2020, 9:57 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
505

Why would that happen?. I tried getting the size of the type stored, the problem is that even for %offload_sizes which is an array of i64 the offset returned is 8 bytes for example, not 64 bytes. That's why I assumed it to be always divided by the size of a pointer instead of the size of the type stored.

jdoerfert added inline comments.Aug 31 2020, 10:11 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
505

it should not happen but it can it the pointer is casted before the store.

hamax97 closed this revision.Aug 31 2020, 2:18 PM

rG8931add61705 [OpenMPOpt][HideMemTransfersLatency] Get values stored in offload arrays.