[PATCH] D80051: [OpenMPOpt] Test case 1 - Latency Hiding for Host to Device Memory Transfers

[Hamilton Tobon-Mosquera via Phabricator via llvm-commits]

hamax97 marked 2 inline comments as done.
hamax97 added a comment.

In D80051#2039931 <https://reviews.llvm.org/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.


Do you think I should create all those test cases before starting to code?. Also, making the API thinner would help a lot I guess. Actually, I don't fully understand what the `baseptr` array is for, could you give me a simple explanation of its usage?.



================
Comment at: llvm/test/Transforms/OpenMP/mem_transfer_hiding.ll:2
+; RUN: opt -S -openmpopt -stats < %s 2>&1
+; REQUIRES: asserts
+
----------------
jdoerfert wrote:
> Why do you want to run this with stats? I guess later you just use filecheck and verify the IR, right?
I thought it would be useful to perhaps count the number of split runtime calls, and how much they were moved forward and backwards. What do you think?. I have no problem not doing it.


================
Comment at: llvm/test/Transforms/OpenMP/mem_transfer_hiding.ll:145
+;}
+define dso_local i32 @heavyComputation2(double* %a, i32 %size) {
+entry:
----------------
jdoerfert wrote:
> For the issue to be hoisted above the `rand` call the pointer needs to be noalias or we need better information about `rand`. As it is, `rand` might modify `a` which prevents moving.
Oh yes, you're right. So, this test case is only useful if `*a` is `noalias`, or I could add some code between the `target data map()` and the `target teams` which would enable moving the `wait` function. This latter way would avoid the use of `noalias` which is not commonly used I guess. But maybe it's a good idea to have a test case where `noalias` is used, it would be a nice optimization.


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

https://reviews.llvm.org/D80051