In D87259#2301065, @grokos wrote:

I think these API functions should also include the source location pointer from https://reviews.llvm.org/D87946. We need to consider renaming the *_issue and *_wait functions to extend the *_loc API the aforementioned patch is introducing. E.g. after D87946 the "current" data begin API function will be __tgt_target_data_begin_mapper_loc, so this patch should extend that name as __tgt_target_data_begin_mapper_loc_issue and __tgt_target_data_begin_mapper_loc_wait. Because both patches make changes to the API, I think it's better to wait until the former patch has been committed.

Ping!. Could you please take a look at it?

Passing handle as input/output argument to __tgt_target_data_begin_mapper_<issue|wait>.

In D87260#2275287, @jdoerfert wrote:

tests?

• Fixed bug when creating the wait runtime call.

• Added constants to ease the access to OffloadArrays in the function detectIssue.

In D87260#2261012, @jdoerfert wrote:

In D87260#2261006, @hamax97 wrote:

In D87260#2260098, @jdoerfert wrote:

Tests?

The regression test for this is hide_mem_transfer_latency.ll. But there's still the issue with the AAManager, so the test has the fixmes of where the set up instructions should be moved.

But it seems not to include any changes to the test. why not? because this doesn't have an impact without resolving the issue you mentioned?

In D87260#2260098, @jdoerfert wrote:

Tests?

• Adding regression test

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

• Removing factory function for OffloadArray, now using it as member function.
• Calculating the accessed index of the OffloadArray using the offset returned by GetPointerBaseWithConstantOffset.

• NIT changes to comments

• NIT changes.
• Tried to split the patch, but then we couldn't test for the values we got.

• Require comments
• Added REQUIRES: assertions to regression test.

NIT: Refactor some variable declarations.

• Adding regression test.

• Removing comparison with explicit runtime functions from canBeMovedDownwards.
• Minor refactors to comments and arguments.

Adding constraint mayReadFromMemory to canBeMovedDownwards.

In D86155#2224292, @jdoerfert wrote:

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

Commit: https://github.com/llvm/llvm-project/commit/ad03d0647fed02f74f811ff09341a3e7a72fdfc6

Commit: https://github.com/llvm/llvm-project/commit/496f8e5b369f091def93482578232da8c6e77a7a

• Refactor to some names and other minor changes.
• Update regression test to expect the split runtime calls.

• Isolating split function.

In D85535#2215750, @jdoerfert wrote:

In D85535#2215712, @hamax97 wrote:

In D85535#2214821, @jdoerfert wrote:

In D85535#2213698, @jdoerfert wrote:

Hm, is it possible to apply this one first. I think this is the easiest to approve right away, that way we have less outstanding code.

WDYT?

But we need to add the structure MemoryTranfer too. Also remember this assumes we moved the declarations in OpenMPOpt.cpp to OpenMPOpt.h, so we would have to put this new code into OpenMPOpt.cpp, which I think is not a problem at all, but just to consider it.

Why do we need anything to split the runtime calls? The arguments should be sufficient for the split, no?

In D85535#2214821, @jdoerfert wrote:

In D85535#2213698, @jdoerfert wrote:

Hm, is it possible to apply this one first. I think this is the easiest to approve right away, that way we have less outstanding code.

WDYT?

• Changing std::vector for SmallVector in split function.

Sorry. Error updating last patch.

• Fix to canBeMovedDownwards

• Fix to canBeMovedDownwards.

In D85840#2213676, @sstefan1 wrote:

Can you add parent revisions for this and maybe other related patches? It will be easier to know the order.

In D80051#2213679, @sstefan1 wrote:

This can be closed, right?

• Removing move code.

In D85535#2213345, @jdoerfert wrote:

Is this a diff against master?

• Splitting patch into canBeMoved logic and splitting the runtime call.
• Fix to minor bug in detectIssue.
• Update regression test. Now checks that the runtime call __tgt_target_data_begin is successfully split. Not sure if this should go in another patch.

• Using OMPIRBuilder instead of manually generating the runtime calls.
• Updating regression test for this optimization.

I think if I only add the declarations of __tgt_target_data_begin_issue and __tgt_target_data_being_wait the linking process will fail. How should I define the wait function?, the issue is just a wrapper of the current function, but as we don't have Shilei's patch merged yet I would have to create something that does nothing and change it later when the asynchronous stuff is merged.

• Fix code style issue.
• Problem with getModRefInfo, it always returns that a value V may be modified, even when running opt with -aa-pipeline=basic-aa.

In D83316#2184765, @JonChesterfield wrote:

I'm not totally sure about this. There's a lot of test setup relative to the test case itself, and I can't work out by reading the test itself what it's checking. What are all the mock classes for?

I'm wondering if we would be better off refactoring the code it intends to test so that it needs less setup.

• Refactors discussed made.
• OpenMPOpt unit tests infrastructure merged.

In D83316#2160625, @jdoerfert wrote:

Wow, this required more machinery than I expected.. thanks!

Should we merge this with the MemoryTransfer logic so we can test it?

Finished getting values in offload arrays

Unit test for getValuesInOfflArrays working.

In D83316#2140024, @jdoerfert wrote:

Should we merge the test header into the test cpp?

We need an actual unit test that runs :)

• Exposing getValuesInOfflArrays() to make it unit testable.

• Adding static function needed in unittest.
• Fixing description in header of new file.

• Fix heavyComputation1 test case.
• Passing all test cases through optimizer in order to remove some unnecessary code.

[WIP]

• Creating the first unit test for OpenMPOpt. This creation led me to move the declaration of OpenMPOpt and OMPInformationCache to OpenMPOpt.h.
• I propose refactoring the optimizations to separate structures so that they are easily maintainable and testable.

Including all diff

Changing from returning status code to bool + Getting MemorySSAAnalysis through OMPInfoCache

Added FileCheck constructs

Added __tgt_target_data_begin to the commented original code, just to make the test cases easier to read.

• Fixed illegal movement: now there's one negative test case (issue cannot be moved) and one positive (with noalias) where the issue can be moved.
• Added data transfer only test case.

In D80051#2039931, @jdoerfert wrote:

This is great! Thanks for starting. I left two comments but this looks pretty good to me.
We will later need positive and negative test cases. We can make those smaller though,
you just have the memory transfer request, no target region. We want to verify we move it
properly, e.g., when it appears in loops, in conditionals, there are memory accesses
before/after, ...

Btw. if it helps, we can also create a "thinner" version of the API that takes less arguments
and is applicable only in some often occurring cases, e.g., we can remove the baseptr array
and use it only if ptr and baseptr have the same entries anyway.

In D76058#1934950, @jdoerfert wrote:

LGTM. Thx!

Removing unnecessary check for variadic function.

Minor change to test file + adding std::move to object copy.