Page MenuHomePhabricator

grokos (George Rokos)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 16 2016, 1:22 PM (332 w, 6 d)

Recent Activity

Jan 27 2022

grokos updated the diff for D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..

Added a check to catch the error when the user tries to map distinct sections of the same object using different pointers.

Jan 27 2022, 6:09 AM · Restricted Project

Jan 26 2022

grokos added inline comments to D118268: [openmp] Disable build of old runtimes by default.
Jan 26 2022, 9:48 AM · Restricted Project

Jan 24 2022

grokos added a comment to D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..

Changed the title because it was totally misleading.

Jan 24 2022, 8:51 AM · Restricted Project
grokos updated the diff for D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..

Added test for use_device_ptr

Jan 24 2022, 8:38 AM · Restricted Project

Jan 23 2022

grokos added inline comments to D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..
Jan 23 2022, 1:07 PM · Restricted Project
grokos added inline comments to D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..
Jan 23 2022, 12:45 PM · Restricted Project
grokos requested review of D117997: [libomptarget] Correctly return the implicit device base address when the base itself has not been mapped..
Jan 23 2022, 10:44 AM · Restricted Project

Dec 6 2021

grokos added a comment to D113124: [OpenMP] Avoid costly shadow map traversals whenever possible.

Ping. I'm preparing some more ShadowPtrMap optimizations which I'd like to base on top of this patch. Is there any planned change or can we land the patch?

Dec 6 2021, 5:17 PM · Restricted Project

Nov 20 2021

grokos added inline comments to D113119: [OpenMP] Introduce asynchronous malloc and free.
Nov 20 2021, 3:22 PM · Restricted Project, Restricted Project

Nov 10 2021

grokos accepted D113124: [OpenMP] Avoid costly shadow map traversals whenever possible.

LGTM with two minor nits.

Nov 10 2021, 1:36 PM · Restricted Project

Nov 3 2021

grokos added a comment to D113119: [OpenMP] Introduce asynchronous malloc and free.

Diff shows rtl.h and device.h have been deleted?

Nov 3 2021, 9:39 AM · Restricted Project, Restricted Project

Oct 25 2021

grokos committed rG2feafa2e460c: [libomptarget][NFC] Add comment explaining why we pass argument bases and (authored by grokos).
[libomptarget][NFC] Add comment explaining why we pass argument bases and
Oct 25 2021, 2:55 PM
grokos added a comment to D112490: [OpenMP][Offloading] Calculate offset in libomptarget.

I see. Thanks for the information. We don't support the case you mentioned in current repo, right? I did check all uses of the offsets in current repo, but looks like they just add them up everywhere.

Oct 25 2021, 2:45 PM · Restricted Project
grokos added a comment to D112490: [OpenMP][Offloading] Calculate offset in libomptarget.

We need to keep bases and offsets separate. Sometimes (e.g. in OpenCL) we need to manifest base pointers prior to launching a kernel. Even if you have mapped an object only partially, e.g. A[N:M], although the kernel is expected to access elements starting at address &A[N] and beyond, we still need to manifest the base of the array &A[0]. In other cases, e.g. the COI API, need the begin address itself, i.e. &A[N] as the API operates on begin addresses, not bases. That's why we pass args and offsets as two separate entities so that each plugin can do what it needs. What you are trying to do with this patch is to revert https://reviews.llvm.org/D33028.

Oct 25 2021, 2:33 PM · Restricted Project
grokos accepted D112475: [OpenMP][Offloading] Only get trip count if team construct.

LGTM

Oct 25 2021, 11:44 AM · Restricted Project

Sep 20 2021

grokos accepted D110104: [OpenMP][NVPTX] Fix a warning that data argument not used by format string.

LG. Such fixes can go in without a review :)

Sep 20 2021, 1:44 PM · Restricted Project

Sep 15 2021

grokos accepted D109846: [OpenMP][libomptarget] Minor fix in x86_64 plugin.

Good catch, LGTM!

Sep 15 2021, 1:48 PM · Restricted Project

Sep 10 2021

grokos accepted D109304: [OpenMP][libomptarget] Add __tgt_target_return_t enum for __tgt_target_XXX return int.
Sep 10 2021, 1:59 PM · Restricted Project
grokos added a comment to D109304: [OpenMP][libomptarget] Add __tgt_target_return_t enum for __tgt_target_XXX return int.

I had another look at this. It seems the assertion is just fine, we would return SUCCESS anyway right after assert(), so we are now double-checking that SUCCESS is indeed the correct value to return.

Sep 10 2021, 12:57 PM · Restricted Project
grokos added a comment to D109619: [openmp][docs] Describe how the internal components are found.

LG, quite descriptive and covers everything related to OpenMP libraries. Waiting for others to comment as well.

Sep 10 2021, 11:35 AM · Restricted Project

Sep 7 2021

grokos accepted D106809: [clang-offload-bundler] Make Bundle Entry ID backward compatible.

LGTM as well.

Sep 7 2021, 1:02 PM · Restricted Project
grokos accepted D109303: [OpenMP][libomptarget][NFC] Change checkDeviceAndCtors return type to bool..

LG. This can be considered NFC, can you put the [NFC] tag in the title?

Sep 7 2021, 11:49 AM · Restricted Project

Sep 5 2021

grokos added inline comments to D109277: [OpenMP][libomptarget] Change internal return status to enum type.
Sep 5 2021, 3:04 PM · Restricted Project

Sep 1 2021

grokos added a comment to D106809: [clang-offload-bundler] Make Bundle Entry ID backward compatible.

LG. One possible suggestion is that you leave the double dash (--) variant in some tests so that we can make sure both variants (e.g. both openmp-amdgcn-amd-amdhsa--gfx906 and openmp-amdgcn-amd-amdhsa-gfx906) are correctly parsed.

Sep 1 2021, 3:47 PM · Restricted Project
grokos added a comment to D109071: [libomptarget] Set runpath on libomptarget, use that to drop LD_LIBRARY_PATH from test runner.

This solution is quite neat and much simpler than D102043, in line with what we want. LGTM, waiting for other people's opinions.

Sep 1 2021, 10:12 AM · Restricted Project
grokos added a comment to D107925: [OpenMP] Use IsHostPtr where needed for targetDataEnd.

I think it's better to move on with these patches and then revisit D108569, which needs extensive changes anyway. It will be easier to work on that other patch if the fixes from this series have landed instead of trying to think about all corner cases all over again.

Sep 1 2021, 9:09 AM · Restricted Project

Aug 31 2021

grokos accepted D109007: [libomptarget] Move HostDataToTargetTy states into StatesTy.
Aug 31 2021, 12:41 PM · Restricted Project
grokos added inline comments to D109007: [libomptarget] Move HostDataToTargetTy states into StatesTy.
Aug 31 2021, 12:06 PM · Restricted Project
grokos added inline comments to D108569: [OpenMP] Enable map checks under unified_shared_memory mode.
Aug 31 2021, 10:29 AM

Aug 28 2021

grokos committed rGa2bd44089e3f: [libomptarget][NFC] Fixed tests which checked for obsolete string… (authored by grokos).
[libomptarget][NFC] Fixed tests which checked for obsolete string…
Aug 28 2021, 7:41 AM

Aug 27 2021

grokos accepted D107928: [OpenMP] Use IsHostPtr where needed in rest of omptarget.cpp.

I don't think it is at all possible to write a test emulating this rare case of TgtPtr in GPU memory having the same value as HstPtr in host memory, so this is good to go even without a test. LGTM.

Aug 27 2021, 3:54 PM · Restricted Project
grokos accepted D107927: [OpenMP] Use IsHostPtr where needed for targetDataBegin.

LG, just update the documentation in device.h:lookupMapping().

Aug 27 2021, 3:51 PM · Restricted Project
grokos accepted D107925: [OpenMP] Use IsHostPtr where needed for targetDataEnd.
Aug 27 2021, 3:12 PM · Restricted Project
grokos accepted D106510: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in runtime (2/2).

LGTM.

Aug 27 2021, 9:49 AM · Restricted Project, Restricted Project
grokos added a comment to D108528: [OpenMP][Offloading] Add support for event related interfaces.

Changing plugin interfaces will be very painful. All changes since 2019 were all to add new interfaces just to make sure not to break existing applications.

Aug 27 2021, 9:47 AM · Restricted Project
grokos accepted D107926: [OpenMP][NFC] Eliminate CopyMember from targetDataEnd.

DelEntry is true only if IsLast is true.

Aug 27 2021, 9:17 AM · Restricted Project

Aug 26 2021

grokos committed rG3819aae6ddae: [libomptarget][NFC] Replaced obsolete name "getOrAllocTgtPtr" with new… (authored by grokos).
[libomptarget][NFC] Replaced obsolete name "getOrAllocTgtPtr" with new…
Aug 26 2021, 6:06 PM
grokos added inline comments to D106510: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in runtime (2/2).
Aug 26 2021, 5:56 PM · Restricted Project, Restricted Project

Aug 23 2021

grokos accepted D108349: [openmp][devicertl] Freestanding nvptx via stub printf.

LGTM too.

Aug 23 2021, 12:53 PM · Restricted Project
grokos added a comment to D108528: [OpenMP][Offloading] Add support for event related interfaces.

Typos. LGTM, waiting for @ye-luo to comment on the patch.

Aug 23 2021, 8:46 AM · Restricted Project

Aug 20 2021

grokos added a comment to D104418: [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement.

There is a performance hit from frequent creating/destroying events at very transfer. The create/destroy logic can be separate from recording events.

Aug 20 2021, 9:24 AM · Restricted Project
grokos accepted D104418: [OpenMP][Offloading] Fixed data race in libomptarget caused by async data movement.

A few minor nits. Generally the patch is now in good shape, its logic is much simpler than the first iteration and it's easy to see that we don't risk running into deadlocks or further races. We haven't been very enthusiastic about alternative approaches; meanwhile there's a race waiting to be fixed, so I think we should move on with providing a solution. LGTM on my side, but since a lot of other people have reviewed the patch, let's wait until we hear them, too.

Aug 20 2021, 2:50 AM · Restricted Project

Aug 19 2021

grokos added inline comments to D108349: [openmp][devicertl] Freestanding nvptx via stub printf.
Aug 19 2021, 2:55 AM · Restricted Project

Aug 13 2021

grokos added a comment to D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2).

I was wondering about the connection to OpenACC, so I had a quick look into the OpenACC spec to try and understand the background.
OpenACC uses two separate reference counters for structured and unstructured map. If one of them is >0, the data is present. If both become 0, data is deleted.

I think, the hold modifier is not sufficient to replicate OpenACC behavior. Consider the following example:

#pragma acc data copy(a)  // structured ref := 1
{
#pragma acc exit data delete(a) // dynamic ref := 0
#pragma acc enter data copyin(a) // dynamic ref := 1
} // structured ref := 0 // no copyout because dynamic ref >0

As I understand this will be translated to the following OpenMP:

#pragma omp target data map(ompx_hold, tofrom:a)  // ref := 1
{
#pragma omp exit data map(delete:a) // ref := 0  // no action because of hold
#pragma omp enter data map(to:a) // ref := 1
} // ref := 0 // perform map from

I don't think, that trying to map the two openacc reference count to a single openmp reference count will work in general.

Aug 13 2021, 2:09 AM · Restricted Project, Restricted Project, Restricted Project

Aug 10 2021

grokos committed rGdf06ec305753: [libomptarget][NFC] Fix compilation issue with GCC (authored by grokos).
[libomptarget][NFC] Fix compilation issue with GCC
Aug 10 2021, 9:48 AM

Aug 6 2021

grokos added inline comments to D105990: [OpenMP][NFC] Simplify targetDataEnd conditions for CopyMember.
Aug 6 2021, 3:21 PM · Restricted Project
grokos added a comment to D105990: [OpenMP][NFC] Simplify targetDataEnd conditions for CopyMember.

Thanks for the review. I'd feel more comfortable with one small simplification at a time. That is, a subsequent patch (which I'm happy to write if you'd like) could make the further simplification you're proposing and adjust targetDataBegin to match. Does that seem reasonable?

Aug 6 2021, 3:16 PM · Restricted Project
grokos accepted D105990: [OpenMP][NFC] Simplify targetDataEnd conditions for CopyMember.

Rebased.

I'm working on the suggested changes for a separate patch. However, seeing D104555's changes to related code in targetDataBegin, I wonder if there's another patch out there that's about to rewrite this code in targetDataEnd too.

Aug 6 2021, 3:15 PM · Restricted Project
grokos added inline comments to D107656: [OpenMP] Use events and taskyield in target nowait task to unblock host threads.
Aug 6 2021, 2:44 PM

Jul 30 2021

grokos accepted D107164: [OpenMP][Offloading] Remove task wait in nowait interfaces.

Now that target nowait operations are handled via hidden helper tasks which make sure that dependencies are satisfied, these calls to __kmpc_omp_taskwait are not only redundant but also hurt performance as they serialize execution unconditionally. LGTM. Can you edit the patch description and explain exactly what you mean and why this patch is needed? Thanks!

Jul 30 2021, 7:37 AM · Restricted Project

Jul 23 2021

grokos accepted D104555: [OpenMP][Offloading] Fix data race in data mapping by using two locks.

This patch looks good now! Let's commit it and then move on with D104418.

Jul 23 2021, 1:09 PM · Restricted Project

Jul 19 2021

grokos added inline comments to D104555: [OpenMP][Offloading] Fix data race in data mapping by using two locks.
Jul 19 2021, 3:54 AM · Restricted Project

Jul 15 2021

grokos added a comment to D105990: [OpenMP][NFC] Simplify targetDataEnd conditions for CopyMember.

I think we can simplify things even further. I have inlined my reasoning. Please double check in case I missed something.

Jul 15 2021, 9:11 AM · Restricted Project

Jul 14 2021

grokos committed rG0c7a4870c5b6: [libomptarget] Keep the Shadow Pointer Map up-to-date (authored by grokos).
[libomptarget] Keep the Shadow Pointer Map up-to-date
Jul 14 2021, 3:24 PM
grokos closed D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.
Jul 14 2021, 3:24 PM · Restricted Project
grokos updated the diff for D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.

Made the comment more detailed to explain why this patch is needed.

Jul 14 2021, 6:26 AM · Restricted Project
grokos added a comment to D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.

No, only when the PTR is deallocated is the entry removed from the map. But in env/base_ptr_ref_count.c the PTR is a global declare target pointer, so it stays in the map for the lifetime of the application.

Jul 14 2021, 2:35 AM · Restricted Project

Jul 13 2021

grokos added a comment to D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.

No. If the old entry exists in ShadowPtrMap then UpdateDevPtr will remain false, so no update will take place. This patch changes that condition: if either the entry is not in the map or it exists but contains stale data, an update should be performed.

Jul 13 2021, 3:58 PM · Restricted Project
grokos added a comment to D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.

@jdenny So it seems we found a valid OpenMP program that suffered from the corner case you warned about in D105812.

Jul 13 2021, 3:47 PM · Restricted Project
grokos requested review of D105947: [libomptarget] Keep the Shadow Pointer Map up-to-date.
Jul 13 2021, 3:46 PM · Restricted Project
grokos added a comment to D104555: [OpenMP][Offloading] Fix data race in data mapping by using two locks.

What about other instances where we have access to the HostDataToTargetMap followed be data transfers, e.g. when removing a mapping and copy data back to the host? Shouldn't we apply the same logic there?

Jul 13 2021, 11:00 AM · Restricted Project
grokos closed D105812: [libomptarget] Update device pointer only if needed.

Closed by commit rGbb0166dc7279: [libomptarget] Update device pointer only if needed (authored by grokos).

Jul 13 2021, 4:35 AM · Restricted Project
grokos committed rGbb0166dc7279: [libomptarget] Update device pointer only if needed (authored by grokos).
[libomptarget] Update device pointer only if needed
Jul 13 2021, 4:23 AM
grokos added inline comments to D105812: [libomptarget] Update device pointer only if needed.
Jul 13 2021, 4:19 AM · Restricted Project

Jul 12 2021

grokos requested review of D105812: [libomptarget] Update device pointer only if needed.
Jul 12 2021, 6:34 AM · Restricted Project

Jul 9 2021

grokos added a comment to D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin.

Right, only the pointer member is updated, no other fields are modified.

Jul 9 2021, 3:57 PM · Restricted Project
grokos added a comment to D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin.

So the expected behavior is what libomptarget currently does. These 2 patches do not change it, so they are good to go.

Jul 9 2021, 3:40 PM · Restricted Project
grokos added inline comments to D105191: [Clang][OpenMP] Add partial support for Static Device Libraries.
Jul 9 2021, 10:29 AM · Restricted Project
grokos accepted D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin.

Thinking about it a bit more, no matter what the answer to my last question is, even the existing version of the library (without D105121 and D104924) will copy the attached object to the device. So these patches result in logically equivalent behavior. @jdenny I think you can proceed with committing them as patches that simplify the code. If we need to take extra care of the corner case in the above example, then we can prepare a new patch that fixes the bug.

Jul 9 2021, 8:54 AM · Restricted Project
grokos added a comment to D105121: [OpenMP] Avoid checking parent reference count in targetDataBegin.

Just like in D104924, the code after this patch is equivalent to the original one with one exception. The question was raised at our telecon a couple of weeks ago but no one was sure what the right answer is:

Jul 9 2021, 8:39 AM · Restricted Project

Jun 30 2021

grokos accepted D104382: [OpenMP][Offloading] Refined return value of `DeviceTy::getOrAllocTgtPtr`.

I think this is good to go now. Thanks! Maybe you can wait for some of the initial reviewers to accept the patch as well or just go ahead and commit.

Jun 30 2021, 4:24 PM · Restricted Project
grokos added inline comments to D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives.
Jun 30 2021, 7:45 AM · Restricted Project, Restricted Project
grokos added inline comments to D93525: [clang-offload-bundler] Add unbundling of archives containing bundled object files into device specific archives.
Jun 30 2021, 5:34 AM · Restricted Project, Restricted Project

Jun 29 2021

grokos accepted D104924: [OpenMP] Avoid checking parent reference count in targetDataEnd.

The new code is equivalent to the old one but clearer and with one fewer lock. Checking the parent struct is a leftover from back when we had a different map interface.

Jun 29 2021, 6:26 AM · Restricted Project

Jun 23 2021

grokos accepted D104560: [OpenMP] Fix delete map type in ref count debug messages.

LGTM, thanks for the patch!

Jun 23 2021, 1:05 AM · Restricted Project

Jun 22 2021

grokos added inline comments to D104560: [OpenMP] Fix delete map type in ref count debug messages.
Jun 22 2021, 2:38 PM · Restricted Project
grokos accepted D104559: [OpenMP] Improve ref count debug messages.
Jun 22 2021, 1:47 PM · Restricted Project
grokos accepted D104580: [NFC][OpenMP][Offloading] Unified the construction of mapping table entry.

LGTM

Jun 22 2021, 1:22 AM · Restricted Project
grokos added inline comments to D104559: [OpenMP] Improve ref count debug messages.
Jun 22 2021, 1:22 AM · Restricted Project

Jun 21 2021

grokos added inline comments to D104559: [OpenMP] Improve ref count debug messages.
Jun 21 2021, 2:56 PM · Restricted Project

Jun 17 2021

grokos added inline comments to D104382: [OpenMP][Offloading] Refined return value of `DeviceTy::getOrAllocTgtPtr`.
Jun 17 2021, 12:47 AM · Restricted Project

Jun 8 2021

grokos accepted D103927: [OpenMP] Add an information flag for device data transfers.
Jun 8 2021, 4:47 PM · Restricted Project
grokos added inline comments to D103927: [OpenMP] Add an information flag for device data transfers.
Jun 8 2021, 2:41 PM · Restricted Project
grokos added inline comments to D103923: [OpenMP] Add thread limit environment variable support to plugins.
Jun 8 2021, 12:36 PM · Restricted Project

May 21 2021

grokos committed rGd0bc04d6b91d: [libomptarget] Fix a bug whereby firstprivates are not copied over to the device (authored by grokos).
[libomptarget] Fix a bug whereby firstprivates are not copied over to the device
May 21 2021, 10:57 AM
grokos closed D102890: [libomptarget] Fix a bug whereby firstprivates are not copied over to the device.
May 21 2021, 10:56 AM · Restricted Project

May 20 2021

grokos requested review of D102890: [libomptarget] Fix a bug whereby firstprivates are not copied over to the device.
May 20 2021, 4:48 PM · Restricted Project

May 6 2021

grokos accepted D102000: [libomptarget] Add support for target memory allocators to cuda RTL.

Looks good (with one naming nit)

May 6 2021, 10:50 AM · Restricted Project

Apr 16 2021

grokos added inline comments to D99803: [openmp] Add OMPT initialization in libomptarget.
Apr 16 2021, 3:26 PM · Restricted Project

Apr 15 2021

grokos added inline comments to D100600: [OpenMP] Add info for device table changes.
Apr 15 2021, 3:32 PM · Restricted Project

Apr 13 2021

grokos added inline comments to D99803: [openmp] Add OMPT initialization in libomptarget.
Apr 13 2021, 10:49 AM · Restricted Project

Apr 6 2021

grokos accepted D99551: [clang-offload-wrapper] Add standard notes for ELF offload images.

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).

Apr 6 2021, 7:19 PM · Restricted Project, Restricted Project, Restricted Project

Mar 25 2021

grokos added a comment to D95483: [OpenMP][WIP] Pass auxiliary YAML along with device image for OpenMP offload..

New implementation is here: https://reviews.llvm.org/D99360

Mar 25 2021, 12:02 PM

Mar 15 2021

grokos added inline comments to D97883: [libomptarget] Add allocator support for target memory.
Mar 15 2021, 11:59 AM · Restricted Project

Mar 13 2021

grokos committed rG2468fdd9af36: [libomptarget] Add allocator support for target memory (authored by grokos).
[libomptarget] Add allocator support for target memory
Mar 13 2021, 3:53 AM
grokos closed D97883: [libomptarget] Add allocator support for target memory.
Mar 13 2021, 3:53 AM · Restricted Project

Mar 12 2021

grokos updated the diff for D97883: [libomptarget] Add allocator support for target memory.

Rabsed and moved targetAllocExplicit() under omptarget.cpp.

Mar 12 2021, 11:21 AM · Restricted Project

Mar 9 2021

grokos updated the summary of D97883: [libomptarget] Add allocator support for target memory.
Mar 9 2021, 11:37 AM · Restricted Project
grokos updated the summary of D97883: [libomptarget] Add allocator support for target memory.
Mar 9 2021, 11:36 AM · Restricted Project
grokos updated the diff for D97883: [libomptarget] Add allocator support for target memory.

Removed __tgt_rtl_data_alloc_explicit; instead we pass an extra kind argument to __tgt_rtl_data_alloc.

Mar 9 2021, 11:33 AM · Restricted Project