Page MenuHomePhabricator

[OpenMPOpt][SplitMemTransfer] Moving the "wait" down
ClosedPublic

Authored by hamax97 on Aug 18 2020, 10:33 AM.

Details

Summary

canBeMovedDownwards checks if the "wait" counterpart of the runtime call can be moved downwards, returning a pointer to the instruction that might require/modify the data transferred, and returning null it the movement is not possible or not worth it. The function splitTargetDataBeginRTC receives that returned instruction and instead of moving the "wait" it creates it at that point.

Diff Detail

Event Timeline

hamax97 created this revision.Aug 18 2020, 10:33 AM
hamax97 requested review of this revision.Aug 18 2020, 10:33 AM

We have to check "mayReadMemory" as well I think.

We have to check "mayReadMemory" as well I think.

Is it really necessary?, I think we only care if the instruction writes to memory. If I add it, now only in heaveComputation1 and dataTransferOnly1 the runtime call is split. This because the immediate instructions after __tgt_target_data_begin_mapper in the other test functions is a load.

We have to check "mayReadMemory" as well I think.

Is it really necessary?, I think we only care if the instruction writes to memory. If I add it, now only in heaveComputation1 and dataTransferOnly1 the runtime call is split. This because the immediate instructions after __tgt_target_data_begin_mapper in the other test functions is a load.

Not splitting in this example is OK. Load is a problem if the transfer is to the issuing device, store is always a problem.

hamax97 updated this revision to Diff 286383.Aug 18 2020, 1:17 PM

Adding constraint mayReadFromMemory to canBeMovedDownwards.

jdoerfert added inline comments.Aug 18 2020, 2:12 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
719

Can't you just check if CurrentI is RuntimeCall?

737

You cannot look for the target calls. What if you encounter a call to a function that contains such a call? However, as long as we don't allow side effects this should not be an issue, just remove this part.

You need to check if it is a call that it has a nosync attribute though.

746

Or something else that looks less confusing.

752

Style: Just make them references instead of pointers.

hamax97 added inline comments.Aug 18 2020, 4:16 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
719

I think it's never RuntimeCall. It starts with the instruction after RuntimeCall.

jdoerfert added inline comments.Aug 18 2020, 4:21 PM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
719

OK, keep it.

hamax97 updated this revision to Diff 286426.Aug 18 2020, 4:37 PM
  • Removing comparison with explicit runtime functions from canBeMovedDownwards.
  • Minor refactors to comments and arguments.
This revision is now accepted and ready to land.Aug 18 2020, 6:40 PM
sstefan1 added inline comments.Aug 19 2020, 12:28 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
717

nit: alias

723

typo: alias

sstefan1 added inline comments.Aug 19 2020, 12:30 AM
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
723

ignore this one, sorry