- User Since
- Feb 16 2016, 1:22 PM (278 w, 2 d)
Tue, Jun 8
Fri, May 21
Thu, May 20
May 6 2021
Looks good (with one naming nit)
Apr 16 2021
Apr 15 2021
Apr 13 2021
Apr 6 2021
Change looks good, so it's accepted on my end. I'll let the other reviewers have a look and post their comments. Please do not commit until we have reached an agreement for all 4 patches together (D99551, D99552, D99553, D99612).
Mar 25 2021
Mar 15 2021
Mar 13 2021
Mar 12 2021
Rabsed and moved targetAllocExplicit() under omptarget.cpp.
Mar 9 2021
Removed __tgt_rtl_data_alloc_explicit; instead we pass an extra kind argument to __tgt_rtl_data_alloc.
Mar 4 2021
Mar 3 2021
Mar 1 2021
OK, clear. You are trying to avoid the call to HandleTargetOutcome(false,...) which erroneously says that offloading has failed. LGTM.
This is a correct observation. The host device is not a target device, so there is no offloading available.
Feb 25 2021
Libomptarget changes look good, I'll let @jdoerfert provide feedback for the clang part.
Feb 24 2021
Feb 18 2021
This change makes much sense. In fact, CUDA 8 was so problematic for use with the nvptx runtime that (if memory serves me well) we declared it unsupported. So essentially this patch drops support for CUDA version 7 (and lower), which is already six years old. If the other reviewers agree, we can accept the patch.
Is this the missing behavior from D83061? Looks good, thanks!
Feb 16 2021
NFC patch, naming in line with the rest of the library, good to go.
Feb 14 2021
Feb 11 2021
Feb 10 2021
Jan 27 2021
I've tested the change and everything seems to work fine. So let's commit it. Thanks for the patch!
Jan 26 2021
Looks good, thanks!
I tried inserting the following forward-declaration right before the function definition and the problem seems to go away:
static inline uint32_t getInfoLevel() __attribute__((used));
I guess it's better to have the above rather than a dummy call?
Jan 25 2021
Jan 12 2021
Also, since D94565 is merged now, this patch needs rebasing :)
One question is, why do we need two libraries? The only difference is, the static library contains omp_data.cu and the bitcode library doesn't. It's unclear why they were implemented in this way
Jan 5 2021
Slight optimization - removed condition from the hot path.
Jan 4 2021
Thanks for the changes! LGTM.
Dec 28 2020
Dec 23 2020
Nov 17 2020
Nov 6 2020
Nov 5 2020
The link in the description for the clang patch is outdated (that patch has been abandoned), can you replace it with https://reviews.llvm.org/D84192? Thanks!
Oct 20 2020
Can we now abandon D75581?
Oct 16 2020
Oct 12 2020
Oct 5 2020
I just revisited this patch. It seems it's based on a very early implementation of the base memory manager patch (D81054). Can you rebase the patch so that we review it? Thanks!
Sep 29 2020
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.
Sep 25 2020
LGTM as well.
Sep 23 2020
Sep 21 2020
Sep 18 2020
Sep 1 2020
Minor comments (about typos) to be taken into account in case of a future patch.
Aug 28 2020
Aug 17 2020
Aug 6 2020
Nice catch! Looks good.
Aug 5 2020
Aug 4 2020
OK, now it makes sense. LGTM
I suppose the same must be applied to all cases where present is used. Without a modification like this, present just confirms that the starting address of the object is already mapped, although a size may be specified in the map clause.
#pragma omp target map(present: p[0:10]) // present will succeed even if only p is mapped and p-p are not
So the question here is: should present apply to the size as well as the begin address? I would say yes.
So does the mapper function emit entries in reverse order upon exiting a target/target data region?
Aug 3 2020
Jul 29 2020
Jul 27 2020
Thanks, it makes sense. Can you submit a patch?
Any idea why this change broke some tests? And how does --crash fix the problem?
This looks much better now. I don't have any other comments. Since this patch is now essentially a clang-only patch, I'll let @ABataev accept it or post comments.
(Earlier I posted here an answer for another patch). Looks good, obviously it must be committed after the clang patch.