[OpenMP] Libomptarget depends on `libffi`

[jhuber6]

We currently have a dependency on libffi in the CPU version of the offloading library. The dependency on libffi is unnecessary and also prevents us from having a unified way to pass arguments to the offloading library. When we emit "kernels" for the CPU they are simply functions in a shared library, see https://godbolt.org/z/5j9r1x9xq.

define weak_odr protected void @__omp_offloading_10302_bd4d1_main_l3(ptr %dyn_ptr, i64 %x) {
entry:
  %dyn_ptr.addr = alloca ptr, align 8
  %x.addr = alloca i64, align 8
  store ptr %dyn_ptr, ptr %dyn_ptr.addr, align 8
  store i64 %x, ptr %x.addr, align 8
  %0 = load i32, ptr %x.addr, align 4
  ret void
}

What we should do instead is emit this kernel with a known argument that is simply a natively packed struct of all the arguments. This would allow us to pass the arguments as simply a pointer and size, similarly to how both CUDA and HSA allow you to pass arguments.

define weak_odr protected void @__omp_offloading_10302_bd4d1_main_l3(ptr noundef %args) {
entry:
  %args.addr = alloca ptr, align 8
  store ptr %args, ptr %args.addr, align 8
  %0 = load ptr, ptr %args.addr, align 8
  %dyn_ptr = getelementptr inbounds %struct.Args, ptr %0, i32 0, i32 0
  %x = getelementptr inbounds %struct.Args, ptr %0, i32 0, i32 8
  ret void
}

The code that generates this prologue is at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L617, which is shared between all captured OpenMP statements.

[llvmbot]

@llvm/issue-subscribers-openmp

Author: Joseph Huber (jhuber6)

We currently have a dependency on `libffi` in the CPU version of the offloading library. The dependency on `libffi` is unnecessary and also prevents us from having a unified way to pass arguments to the offloading library. When we emit "kernels" for the CPU they are simply functions in a shared library, see https://godbolt.org/z/5j9r1x9xq.

define weak_odr protected void @<!-- -->__omp_offloading_10302_bd4d1_main_l3(ptr %dyn_ptr, i64 %x) {
entry:
  %dyn_ptr.addr = alloca ptr, align 8
  %x.addr = alloca i64, align 8
  store ptr %dyn_ptr, ptr %dyn_ptr.addr, align 8
  store i64 %x, ptr %x.addr, align 8
  %0 = load i32, ptr %x.addr, align 4
  ret void
}

What we should do instead is emit this kernel with a known argument that is simply a natively packed struct of all the arguments. This would allow us to pass the arguments as simply a pointer and size, similarly to how both CUDA and HSA allow you to pass arguments.

define weak_odr protected void @<!-- -->__omp_offloading_10302_bd4d1_main_l3(ptr noundef %args) {
entry:
  %args.addr = alloca ptr, align 8
  store ptr %args, ptr %args.addr, align 8
  %0 = load ptr, ptr %args.addr, align 8
  %dyn_ptr = getelementptr inbounds %struct.Args, ptr %0, i32 0, i32 0
  %x = getelementptr inbounds %struct.Args, ptr %0, i32 0, i32 8
  ret void
}

The code that generates this prologue is at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L617, which is shared between all captured OpenMP statements.

[jhuber6]

@alexey-bataev I'm not overly familiar with how this should look from the clang side. Looking at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L617 I'm thinking that all we need to do is adjust the function type and arguments. The function is created here https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L525, but I don't know what the rules are. Ideally I could just combine all the arguments into a struct and then provide the same interface by getting the arguments from a GEP, but I don't know how to do that given the Clang interface. Some pointers would be appreciated.

[alexey-bataev]

@alexey-bataev I'm not overly familiar with how this should look from the clang side. Looking at https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L617 I'm thinking that all we need to do is adjust the function type and arguments. The function is created here https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGStmtOpenMP.cpp#L525, but I don't know what the rules are. Ideally I could just combine all the arguments into a struct and then provide the same interface by getting the arguments from a GEP, but I don't know how to do that given the Clang interface. Some pointers would be appreciated.

You need to build the artificial record and put all the parameters to the member fields. Then need to adjust the type of the function and pass this record argument. Check e9c7291, looks like it is something you want to do

[jhuber6]

@alexey-bataev I looked into that patch and got it working for the basic captured context, however a (semi) recent patch added a forced argument to every target region in

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 4270 in 35e6bae

ParamsTarget.push_back(std::make_pair(StringRef("dyn_ptr"), VoidPtrTy));

. This now shows up as a separate function argument. The approach you linked relies on using the RecordDecl for the __context variable in the captured region to handle the arguments. However, because of the dyn_ptr there will always be one argument that's not in the struct.

How would you get around this? I can't make a new RecordDecl inside of CGOpenMPRuntime because all of the declarations are expected to be constant. I also can't just create a new variable in the scope because this thing needs to be the first argument.

[alexey-bataev]

@alexey-bataev I looked into that patch and got it working for the basic captured context, however a (semi) recent patch added a forced argument to every target region in

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 4270 in 35e6bae

ParamsTarget.push_back(std::make_pair(StringRef("dyn_ptr"), VoidPtrTy));

. This now shows up as a separate function argument. The approach you linked relies on using the RecordDecl for the __context variable in the captured region to handle the arguments. However, because of the dyn_ptr there will always be one argument that's not in the struct.
How would you get around this? I can't make a new RecordDecl inside of CGOpenMPRuntime because all of the declarations are expected to be constant. I also can't just create a new variable in the scope because this thing needs to be the first argument.

You can try to add it into the __context variable as a part of the captured recorddecl

[jhuber6]

You can try to add it into the __context variable as a part of the captured recorddecl

How would that be done? I looked into the surrounding code and couldn't find an easy place to do that. I know we have these Start and End functions for the surrounding scope, then there's some code that actually captures the things in the block scope. There's something like createCapture used elsewhere in the file but I couldn't find how that would be added to the current record decl that's being created. Maybe I'm just missing some accessor that lets us get to the record decl once we call ActOnCapturedRegionStart?

[alexey-bataev]

You can try to add it into the __context variable as a part of the captured recorddecl

How would that be done? I looked into the surrounding code and couldn't find an easy place to do that. I know we have these Start and End functions for the surrounding scope, then there's some code that actually captures the things in the block scope. There's something like createCapture used elsewhere in the file but I couldn't find how that would be added to the current record decl that's being created. Maybe I'm just missing some accessor that lets us get to the record decl once we call ActOnCapturedRegionStart?

Check SemaOpenMP::ActOnOpenMPRegionStart, it adds several fields to the record, like gtid, btid, etc.

[jhuber6]

Check SemaOpenMP::ActOnOpenMPRegionStart, it adds several fields to the record, like gtid, btid, etc.

Code like this

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 4251 in 35e6bae

std::make_pair(".global_tid.", KmpInt32Ty),

?

I think this is the same handling that the dyn_ptr argument is using. These are an entirely different param to the captured region and aren't put into the __context struct. It shows up like this in the AST:

CapturedDecl 0x56065cbc56d8 <<invalid sloc>> <invalid sloc> nothrow
|-ImplicitParamDecl 0x56065cbc5740 <line:6:1> col:1 implicit dyn_ptr 'void *const restrict' 
`-ImplicitParamDecl 0x56065cbd9030 <col:1> col:1 implicit __context 'struct (unnamed at input.c:6:1) *const restrict'

[alexey-bataev]

Check SemaOpenMP::ActOnOpenMPRegionStart, it adds several fields to the record, like gtid, btid, etc.

Code like this

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 4251 in 35e6bae

std::make_pair(".global_tid.", KmpInt32Ty),

?
I think this is the same handling that the dyn_ptr argument is using. These are an entirely different param to the captured region and aren't put into the __context struct. It shows up like this in the AST:

CapturedDecl 0x56065cbc56d8 <<invalid sloc>> <invalid sloc> nothrow
|-ImplicitParamDecl 0x56065cbc5740 <line:6:1> col:1 implicit dyn_ptr 'void *const restrict' 
`-ImplicitParamDecl 0x56065cbd9030 <col:1> col:1 implicit __context 'struct (unnamed at input.c:6:1) *const restrict'

Ah, ok, you want to make it part of the __context? Then you need to build an artificial variable in the outer scope and then capture it in the OpenMP context using SemaRef.MarkVariableReferenced. Check SemaOpenMP.cpp, lines 4954-4969, it has something similar I assume

[jhuber6]

Ah, ok, you want to make it part of the __context? Then you need to build an artificial variable in the outer scope and then capture it in the OpenMP context using SemaRef.MarkVariableReferenced. Check SemaOpenMP.cpp, lines 4954-4969, it has something similar I assume

I tried putting this in place of where we currently handle the dyn_ptr but it doesn't show up in the __context struct. You mentioned "OuterScope" which I interpreted to be creating this before we call ActOnCapturedRegionStart. Is there somewhere else I need to stick this so the scope recognizes it as captured?

auto *VD = buildVarDecl(SemaRef, DSAStack->getConstructLoc(), VoidPtrTy,
                        "dyn_ptr");
auto *RE = buildDeclRefExpr(SemaRef, VD, VD->getType(), VD->getLocation());
SemaRef.MarkVariableReferenced(VD->getLocation(), VD);

[alexey-bataev]

Ah, ok, you want to make it part of the __context? Then you need to build an artificial variable in the outer scope and then capture it in the OpenMP context using SemaRef.MarkVariableReferenced. Check SemaOpenMP.cpp, lines 4954-4969, it has something similar I assume

I tried putting this in place of where we currently handle the dyn_ptr but it doesn't show up in the __context struct. You mentioned "OuterScope" which I interpreted to be creating this before we call ActOnCapturedRegionStart. Is there somewhere else I need to stick this so the scope recognizes it as captured?

auto *VD = buildVarDecl(SemaRef, DSAStack->getConstructLoc(), VoidPtrTy,
                        "dyn_ptr");
auto *RE = buildDeclRefExpr(SemaRef, VD, VD->getType(), VD->getLocation());
SemaRef.MarkVariableReferenced(VD->getLocation(), VD);

It should be created before call of ActOnCapturedRegionStart, but marked as referenced after ActOnCapturedRegionStart (and before ActOnCapturedRegionEnd()).

[jhuber6]

It should be created before call of ActOnCapturedRegionStart, but marked as referenced after ActOnCapturedRegionStart (and before ActOnCapturedRegionEnd()).

Okay, that makes sense. I moved that after the scope beginning and hit an assertion. Guessing that I need to register this in the map somehow?

DeclRefExpr for Decl not entered in LocalDeclMap?
UNREACHABLE executed at /home/jhuber/Documents/llvm/llvm-project/clang/lib/CodeGen/CGExpr.cpp:3086!

(Thanks for walking me through this, much of clang is a mystery to me).

[alexey-bataev]

It should be created before call of ActOnCapturedRegionStart, but marked as referenced after ActOnCapturedRegionStart (and before ActOnCapturedRegionEnd()).

Okay, that makes sense. I moved that after the scope beginning and hit an assertion. Guessing that I need to register this in the map somehow?

DeclRefExpr for Decl not entered in LocalDeclMap?
UNREACHABLE executed at /home/jhuber/Documents/llvm/llvm-project/clang/lib/CodeGen/CGExpr.cpp:3086!

(Thanks for walking me through this, much of clang is a mystery to me).

Yes, you need to create an artificial DeclStmt statement and emit code for it to make this variable defined in the function scope. Or emit it yourself somehow.

[jhuber6]

Yes, you need to create an artificial DeclStmt statement and emit code for it to make this variable defined in the function scope. Or emit it yourself somehow.

I see some code where we create a new DeclStmt like here

llvm-project/clang/lib/Sema/SemaOpenMP.cpp

Line 9804 in 35e6bae

static Stmt *buildPreInits(ASTContext &Context,

.

Do I simply need to create one for myself with the single variable as the list of declarations?