This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][Offloading] Calculate offset in libomptarget
AbandonedPublic

Authored by tianshilei1992 on Oct 25 2021, 2:11 PM.

Details

Summary

When invoking a kernel function, we pass arguments and offsets to plugin for now.
In each plugin, it recalculates the real argument by adding offsets to arguments.
We can actually move this part to libomptarget.

Diff Detail

Event Timeline

tianshilei1992 created this revision.Oct 25 2021, 2:11 PM
tianshilei1992 requested review of this revision.Oct 25 2021, 2:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 2:11 PM
grokos added a comment.EditedOct 25 2021, 2:33 PM

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.

tianshilei1992 abandoned this revision.Oct 25 2021, 2:42 PM

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]. That's why we pass args and offsets as two separate entities. What you are trying to do with this patch is to revert https://reviews.llvm.org/D33028.

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.

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.

That's right, every plugin we currently have in the open-source repo operates on bases, that's why it's not clear why we separated args and offsets a few years ago. Maybe we should add a comment in the code pointing to D33028 to keep things clear.