Page MenuHomePhabricator

[RFC][POC] Introduce callback argument encoding mode into callback metadata
Needs ReviewPublic

Authored by tianshilei1992 on Mar 6 2021, 8:07 PM.

Details

Summary

Current !callback metadata assumes that arguments of callback function
are encoded in broker function arguments, as the following example shows:

void callback(int, float, double);
void broker(void(*cb)(int, float, double), int, float, double);

We call the argument encoding as "flat mode". However, it is not always the
case. For some programming model, such as CUDA and OpenMP offloading, arguments
are "stacked":

void callback(int *, float *, double *);
void broker(void(*cb)(int *, float *, double *), void **args, size_t size) {
  // ...
  cb(args[0], args[1], args[2]);
}

Let's call this encode as "stacked mode" (a more approporiate name is welcomed).
Current !callback metadata cannot handle this case very well. In fact, we can
establish connection between callback function arguments and args passed to
the broker function via the use chain of args. For example, before we call the
broker function, we need to construct args by setting its every element. In
this way, we can know which pointer eventually corresponds to which callback
function argument.

In order to do that, we need to tell apart the two different parameter encoding
modes. In this patch, the !callback metadata is updated in the following way:

  1. The first operand is an i64 value, representing parameter encoding mode. 0 is "flat mode", and 1 is "stacked mode".
  2. The second operand is the callback function index, same as the previous first operand.
  3. The next one or more operands are still parameter encoding with a little difference. If the encoding mode is 0, it is same as current way. If the mode is 1, there must only be one operand: the index of the base pointer (e.g. the index of void **args above).
  4. The last operand is still for the variadic arguments.

P.S. People might argue in CUDA or OpenMP offloading, the broker function
actually doesn't accept a callback function pointer. It's usually a global
symbol which can be used to find the entry kernel function during the runtime.
More important, the kernel function (or "callback" function) cannot be seen in
the host module at all. What's the point of doing this? Yes, that's true for now.
We're working on the heterogenous module, which basically will "merge" (or "link")
kernel moduel and host module together. In this way, we can optimize them with
the information from both host and kernel side. We can bridge the kernel function
and broker function via the global symbol somehow to make the kernel function a
"virtual" callback function. Although pointers in void **args will not be used
by the kernel function directly (as shown in the example above), the real
arguments passed to the kernel function are 1:1 mapped from pointers in args.
Therefore the correspondence still exists.

Diff Detail

Unit TestsFailed

TimeTest
2,720 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

tianshilei1992 created this revision.Mar 6 2021, 8:07 PM
tianshilei1992 requested review of this revision.Mar 6 2021, 8:07 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
tianshilei1992 added inline comments.Mar 6 2021, 8:09 PM
clang/lib/Sema/SemaDeclAttr.cpp
3642

Need to revise this line to see which is the right number passed here.

llvm/include/llvm/IR/AbstractCallSite.h
55

I guess this enum should be in a more common place such that other files can use it. Any suggestion?

tianshilei1992 edited the summary of this revision. (Show Details)Mar 6 2021, 8:10 PM
tianshilei1992 edited the summary of this revision. (Show Details)

update doc in clang

tianshilei1992 edited the summary of this revision. (Show Details)Mar 8 2021, 4:50 PM

update llvm doc

tianshilei1992 retitled this revision from [WIP][RFC] Introduce callback argument encoding mode into callback metadata to [RFC] Introduce callback argument encoding mode into callback metadata.Mar 8 2021, 7:26 PM

put the encoding mode into ParameterEncoding

add the support for stacked mode in AbstractCallSite::getCallArgOperand

tianshilei1992 retitled this revision from [RFC] Introduce callback argument encoding mode into callback metadata to [RFC][POC] Introduce callback argument encoding mode into callback metadata.Mar 12 2021, 6:49 PM
tianshilei1992 edited the summary of this revision. (Show Details)

We probably don't need the index for size_t size. If the ArgNo is out of range, we simply return nullptr. Besides, CUDA function cudaLaunchKernel doesn't have an argument for size as well.

tianshilei1992 added inline comments.Mar 12 2021, 6:52 PM
llvm/lib/IR/AbstractCallSite.cpp
208

If we don't use have a size, GEP will be nullptr here, and we can just return it.

remove the requirement of size_t size

tianshilei1992 edited the summary of this revision. (Show Details)Mar 13 2021, 11:57 AM
aaron.ballman added inline comments.Mar 15 2021, 5:39 AM
clang/include/clang/Basic/AttrDocs.td
432

It looks like a bunch of unrelated whitespace changes snuck in.

5211

Rather than use 0 and 1 directly, any reason not to use a named enumerator so that the user can write: __attribute__((callback(stacked, 3, 4)))?

Also, should this attribute argument be the last argument in the list and made optional so that existing code will continue to work?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2940

I'm not certain it's reasonable to turn this into an error given that this attribute already exists in the wild. This basically breaks all users of the attribute.