This is an archive of the discontinued LLVM Phabricator instance.

[libomptarget] Add support for target update non-contiguous
ClosedPublic

Authored by cchen on Jun 19 2020, 3:14 PM.

Details

Summary

This patch is the runtime support for https://reviews.llvm.org/D84192.

In order not to modify the tgt_target_data_update information but still be
able to pass the extra information for non-contiguous map item (offset,
count, and stride for each dimension), this patch overload arg when
the maptype is set as OMP_TGT_MAPTYPE_DESCRIPTOR. The origin arg is for
passing the pointer information, however, the overloaded arg is an
array of descriptor_dim:

struct descriptor_dim {
  int64_t offset;
  int64_t count;
  int64_t stride
};

and the array size is the dimension size. In addition, since we
have count and stride information in descriptor_dim, we can replace/overload the
arg_size parameter by using dimension size.

Diff Detail

Event Timeline

cchen created this revision.Jun 19 2020, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 3:14 PM

The test?

openmp/libomptarget/include/omptarget.h
128–130

clang part relies on unsigned types.

cchen updated this revision to Diff 272529.Jun 22 2020, 1:03 PM

Add test

ABataev added inline comments.Jun 22 2020, 1:06 PM
openmp/libomptarget/test/offloading/non_contiguous_update.c
2

We don't have support for this in clang yet, better to add a test that uses runtime calls directly for the new features.

cchen marked an inline comment as done.Jun 22 2020, 1:23 PM
cchen added inline comments.
openmp/libomptarget/test/offloading/non_contiguous_update.c
2

Is there any test file that I can refer to? The run line I wrote here is basically copied from other test files under libomptarget/test.

ABataev added inline comments.Jun 22 2020, 1:36 PM
openmp/libomptarget/test/offloading/non_contiguous_update.c
2

Sure, check llvm-project/openmp/libomptarget/test/mapping/declare_mapper_api.cpp for example

cchen updated this revision to Diff 273134.Jun 24 2020, 12:59 PM

Update test

cchen updated this revision to Diff 273140.Jun 24 2020, 1:06 PM

Revert llvm pre-merge lint for existing code

cchen updated this revision to Diff 273161.Jun 24 2020, 2:26 PM

Update test so that it don't depends on compiler generated code

cchen updated this revision to Diff 273494.Jun 25 2020, 1:10 PM

Fix non_contig type

cchen added a project: Restricted Project.
cchen updated this revision to Diff 279327.Jul 20 2020, 12:39 PM

Modify test for supporting stride in target update non-contiguous

cchen added a comment.Jul 24 2020, 9:16 AM

Ping, compiler support is in this patch: https://reviews.llvm.org/D84192.

grokos added inline comments.Aug 4 2020, 12:19 PM
openmp/libomptarget/include/omptarget.h
53

Maybe use a more descriptive name for the flag? Like OMP_TGT_MAPTYPE_NON_CONTIG?

openmp/libomptarget/src/omptarget.cpp
571

Maybe rename dim to current_dim - if it wasn't for the comment in the function call in line 629 I wouldn't have figured out what this argument describes.

579

alreay --> already
peice --> piece

584

return --> returns

cchen updated this revision to Diff 303194.Nov 5 2020, 11:02 AM

Rebase and fix based on feedback

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!

cchen edited the summary of this revision. (Show Details)Nov 5 2020, 1:24 PM
cchen added a comment.Nov 5 2020, 2:04 PM

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!

I just updated the link, thanks!

grokos accepted this revision.Nov 6 2020, 4:22 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Nov 6 2020, 4:22 PM
This revision was landed with ongoing or failed builds.Nov 6 2020, 6:55 PM
This revision was automatically updated to reflect the committed changes.
tianshilei1992 reopened this revision.Nov 8 2020, 5:33 PM
tianshilei1992 added a subscriber: tianshilei1992.

This patch breaks the building of OpenMP. Please fix it.

FAILED: libomptarget/src/CMakeFiles/omptarget.dir/omptarget.cpp.o
/home/shiltian/Documents/deploy/llvm/release/bin/clang++  -DOMPTARGET_DEBUG -Domptarget_EXPORTS -I/home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/include -Wall -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-extra -Wno-pedantic -std=gnu++14 -g -fPIC -MD -MT libomptarget/src/CMakeFiles/omptarget.dir/omptarget.cpp.o -MF libomptarget/src/CMakeFiles/omptarget.dir/omptarget.cpp.o.d -o libomptarget/src/CMakeFiles/omptarget.dir/omptarget.cpp.o -c /home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/src/omptarget.cpp
/home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/src/omptarget.cpp:664:8: error: use of undeclared identifier 'arg_sizes'
       arg_sizes[i], DPxPTR(TgtPtrBegin), DPxPTR(HstPtrBegin));
       ^
/home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/src/omptarget.cpp:664:18: error: use of undeclared identifier 'i'
       arg_sizes[i], DPxPTR(TgtPtrBegin), DPxPTR(HstPtrBegin));
                 ^
/home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/src/omptarget.cpp:691:8: error: use of undeclared identifier 'arg_sizes'
       arg_sizes[i], DPxPTR(HstPtrBegin), DPxPTR(TgtPtrBegin));
       ^
/home/shiltian/Documents/vscode/llvm-project/openmp/libomptarget/src/omptarget.cpp:691:18: error: use of undeclared identifier 'i'
       arg_sizes[i], DPxPTR(HstPtrBegin), DPxPTR(TgtPtrBegin));
                 ^
4 errors generated.

What's more, please follow LLVM coding style.

openmp/libomptarget/src/omptarget.cpp
514

arg_size is not defined here.

This revision is now accepted and ready to land.Nov 8 2020, 5:33 PM
tianshilei1992 requested changes to this revision.Nov 8 2020, 5:34 PM
This revision now requires changes to proceed.Nov 8 2020, 5:34 PM
cchen updated this revision to Diff 303901.Nov 9 2020, 9:21 AM

Fix build error

tianshilei1992 added inline comments.Nov 9 2020, 9:28 AM
openmp/libomptarget/src/omptarget.cpp
488

Since you're modifying this function, could you please change the coding style to LLVM style as well?

cchen added inline comments.Nov 9 2020, 9:43 AM
openmp/libomptarget/src/omptarget.cpp
488

By coding style do you mean the name of the parameters? Can you be more specific? Thanks.

tianshilei1992 added inline comments.Nov 9 2020, 10:01 AM
openmp/libomptarget/src/omptarget.cpp
488

Yes, sort of. You can refer to https://llvm.org/docs/CodingStandards.html for more details.

cchen updated this revision to Diff 303917.Nov 9 2020, 10:09 AM

Fix coding style

Please revert your commit while you work on a fix, as it breaks building the runtime code.

Fix coding style

I'm not sure whether this is still WIP because I didn't see any change in the touched code.

Fix coding style

I'm not sure whether this is still WIP because I didn't see any change in the touched code.

I've changed the name of the argument in that function and apply clang-format on the function. Is there any place that I'm not following the coding style? I'm just waiting for acceptance to merge the patch.

Fix coding style

I'm not sure whether this is still WIP because I didn't see any change in the touched code.

I've changed the name of the argument in that function and apply clang-format on the function. Is there any place that I'm not following the coding style? I'm just waiting for acceptance to merge the patch.

Take the inline comment as an example, but for those C functions, just leave them what they're now.

openmp/libomptarget/src/omptarget.cpp
488

Based on LLVM standard, this function should be

static int targetDataContiguous(DeviceTy &Device, void *ArgsBase, void *Arg, int64_t argSize, int64_t ArgType)
488
static int targetDataContiguous(DeviceTy &Device, void *ArgsBase, void *Arg, int64_t ArgSize, int64_t ArgType)
cchen updated this revision to Diff 305887.Nov 17 2020, 1:02 PM

Apply LLVM coding style

tianshilei1992 added inline comments.Nov 17 2020, 5:02 PM
openmp/libomptarget/include/omptarget.h
127

Like the void *Queue in __tgt_async_info

openmp/libomptarget/src/omptarget.cpp
515

Still not LLVM style.

521

ditto

522

ditto

524

ditto

cchen updated this revision to Diff 306221.Nov 18 2020, 2:07 PM

Update style

Would you please take a thorough check with your patch? I can still find many code that still don't conform with the standard. Thanks. The test case is fine as we don't have unified standard.

openmp/libomptarget/src/omptarget.cpp
487–488

This function does not conform with the standard.

cchen added a comment.Nov 19 2020, 7:05 AM

Would you please take a thorough check with your patch? I can still find many code that still don't conform with the standard. Thanks. The test case is fine as we don't have unified standard.

Sure, I'm just not sure when to follow the style since the style in this codebase is so inconsistent and it confused if I should modify code not written by me.

cchen updated this revision to Diff 306427.Nov 19 2020, 8:48 AM

Refactor target_data_update

tianshilei1992 accepted this revision.Nov 19 2020, 8:52 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Nov 19 2020, 8:52 AM
This revision was landed with ongoing or failed builds.Nov 19 2020, 9:33 AM
This revision was automatically updated to reflect the committed changes.