[OpenMP] Add target task alloc function with device ID

[gtbercea]

Summary

Add a new call to Clang to perform task allocation for the target.

[Hahnfeld]

As for D63010, the new function isn't used anywhere (and hence cannot be tested). What's the advantage of splitting changes at such granularity?

[AlexEichenberger]

lib/CodeGen/CGOpenMPRuntime.cpp

610

device_id is int64_t

1928

device_id must be a int64_t type, not a machine dependent quantity.

[ABataev]

This patches does not add any functionality. I thought, it was published by an accident. The patch should be abandoned or reworked to add a functional part of the code.

[gtbercea]

• Add temporary implementation.

[gtbercea]

• Add tests.

[gtbercea]

• Add device ID if available.

[gtbercea]

• Fix tests.

[AlexEichenberger]

LGTM

[ABataev]

lib/CodeGen/CGOpenMPRuntime.cpp

1921

The comment is not correct, fix the function name

5121

Can we use the same function in both modes, with nowait clause and without?

[gtbercea]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

For nowait we need to use the target_task_alloc variant. There are runtimes - other than the open source one - for which this function does something different.

[gtbercea]

• Fix function name.

[ABataev]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Trunk relies only to libomptarget interfaces. Why we should take into account some other runtime libraries? Plus, what's so different for async and non-async versions of the directive?

[gtbercea]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Async is not yet fully supported in the OpenMP open source runtime but at some point it will be. This is the first step towards its support. I'm not sure what your objection is. The difference is clear. Device ID must be passed on the async version of this call.

[ABataev]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

The difference is not obvious. Why we can't use the same function for sync directives? The fact the it has DeviceId parameter is not an argument here. What's so special from functional point of view?

[gtbercea]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

When you have multiple device on the same system you have to be able to distinguish between then and manage dependencies across these different devices. Knowing the device is a crucial first step in handling these inter-device dependencies.

[ABataev]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Ok, this is why we need it for async version. But why we can't use for sync version?

[gtbercea]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Based on how the runtime is currently structured, nowait functions are separate from the synchronous ones. See target vs. target_nowait functions and there are many examples like that. The async support is always kept separate from the synchronous support at least in terms of entry points in the runtime.

[AlexEichenberger]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Right now, target_task indicates two things: 1) there are depenences to offload and 2) it's a nowait constructs. If we use target_task_alloc for both situations, we need to distinguish these two cases using flags, because the handling iseventually different.
If we want to explore this avenue, then we have to look into using the proxy flag is set to indicates nowait. It may have implications for the tasks on the KMP side, so this might be more risky.
This is why I suggest at the present time that we only generate target task for the limited "target task depend nowait"

[gtbercea]

lib/CodeGen/CGOpenMPRuntime.cpp

5121

Thanks a lot for the comment @AlexEichenberger

[Hahnfeld]

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

If I had to guess this is because some directives have the device clause, so the tests should check that the generated code passes the correct argument in that case.

[ABataev]

In D63009#1544758, @Hahnfeld wrote:

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

If I had to guess this is because some directives have the device clause, so the tests should check that the generated code passes the correct argument in that case.

+1. Missed the test checks, tests must be fixed

[ABataev]

The tests must check the device ID for target-based calls of the task alloc function.

[gtbercea]

In D63009#1544784, @ABataev wrote:

The tests must check the device ID for target-based calls of the task alloc function.

Since device ID is now an expression I can only do this:

i64 {{.*}})

Is this what you wanted? @ABataev @Hahnfeld ?

[gtbercea]

In D63009#1544758, @Hahnfeld wrote:

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

When I was just passing the default value the LLVM-IR was: i64 -1 i.e. constant, easy to check.

With the latest change the emitted code is: i64 %123 i.e. where %123 is a local derived from the expression of the device ID.

[ABataev]

In D63009#1544900, @gtbercea wrote:

In D63009#1544758, @Hahnfeld wrote:

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

When I was just passing the default value the LLVM-IR was: i64 -1 i.e. constant, easy to check.

With the latest change the emitted code is: i64 %123 i.e. where %123 is a local derived from the expression of the device ID.

If the value is constant, check for the constant. And at least several tests with the expressions should check for the correct value of the expression.

[gtbercea]

In D63009#1544984, @ABataev wrote:

In D63009#1544900, @gtbercea wrote:

In D63009#1544758, @Hahnfeld wrote:

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

When I was just passing the default value the LLVM-IR was: i64 -1 i.e. constant, easy to check.

With the latest change the emitted code is: i64 %123 i.e. where %123 is a local derived from the expression of the device ID.

If the value is constant, check for the constant.

All the tests here use expressions so the value is never constant. If you'd like me to add a test with constant I can.

And at least several tests with the expressions should check for the correct value of the expression.

I'll have to check how to do this. There's nothing that distinguishes an expression that represents the device ID from any other expression in the code.

[ABataev]

In D63009#1545152, @gtbercea wrote:

In D63009#1544984, @ABataev wrote:

In D63009#1544900, @gtbercea wrote:

In D63009#1544758, @Hahnfeld wrote:

Am I correct that the second to last revision ("- Fix tests.") removed all checks for the actual device_id argument from the tests? From my point of view that's not fixing but weakening the tests! Can you explain why they needed "fixing"?

When I was just passing the default value the LLVM-IR was: i64 -1 i.e. constant, easy to check.

With the latest change the emitted code is: i64 %123 i.e. where %123 is a local derived from the expression of the device ID.

If the value is constant, check for the constant.

All the tests here use expressions so the value is never constant. If you'd like me to add a test with constant I can.

Yes, there must be at least one test with the default value.

And at least several tests with the expressions should check for the correct value of the expression.

I'll have to check how to do this. There's nothing that distinguishes an expression that represents the device ID from any other expression in the code.

[gtbercea]

Fixed in D63454

lib/CodeGen/CGOpenMPRuntime.cpp

enum OpenMPRTLFunction {
   // Call to kmp_int32 __kmpc_single(ident_t *, kmp_int32 global_tid);
   OMPRTL__kmpc_single,
   // Call to void __kmpc_end_single(ident_t *, kmp_int32 global_tid);
   OMPRTL__kmpc_end_single,
   // Call to kmp_task_t * __kmpc_omp_task_alloc(ident_t *, kmp_int32 gtid,
   // kmp_int32 flags, size_t sizeof_kmp_task_t, size_t sizeof_shareds,
   // kmp_routine_entry_t *task_entry);
   OMPRTL__kmpc_omp_task_alloc,
+  // Call to kmp_task_t * __kmpc_omp_target_task_alloc(ident_t *,
+  // kmp_int32 gtid, kmp_int32 flags, size_t sizeof_kmp_task_t,
+  // size_t sizeof_shareds, kmp_routine_entry_t *task_entry,
+  // size_t device_id);
+  OMPRTL__kmpc_omp_target_task_alloc,
   // Call to kmp_int32 __kmpc_omp_task(ident_t *, kmp_int32 gtid, kmp_task_t *
   // new_task);
   OMPRTL__kmpc_omp_task,
   // Call to void __kmpc_copyprivate(ident_t *loc, kmp_int32 global_tid,
   // size_t cpy_size, void *cpy_data, void(*cpy_func)(void *, void *),
   // kmp_int32 didit);
   OMPRTL__kmpc_copyprivate,
   // Call to kmp_int32 __kmpc_reduce(ident_t *loc, kmp_int32 global_tid,
case OMPRTL__kmpc_omp_task_alloc: {
     llvm::Type *TypeParams[] = {getIdentTyPointerTy(), CGM.Int32Ty, CGM.Int32Ty,
                                 CGM.SizeTy, CGM.SizeTy, KmpRoutineEntryPtrTy};
     // Return void * and then cast to particular kmp_task_t type.
     auto *FnTy =
         llvm::FunctionType::get(CGM.VoidPtrTy, TypeParams, /*isVarArg=*/false);
     RTLFn = CGM.CreateRuntimeFunction(FnTy, /*Name=*/"__kmpc_omp_task_alloc");
     break;
   }
+  case OMPRTL__kmpc_omp_target_task_alloc: {
+    // Build kmp_task_t *__kmpc_omp_task_alloc(ident_t *, kmp_int32 gtid,
+    // kmp_int32 flags, size_t sizeof_kmp_task_t, size_t sizeof_shareds,
+    // kmp_routine_entry_t *task_entry, size_t device_id);
+    assert(KmpRoutineEntryPtrTy != nullptr &&
+           "Type kmp_routine_entry_t must be created.");
+    llvm::Type *TypeParams[] = {getIdentTyPointerTy(), CGM.Int32Ty, CGM.Int32Ty,
+                                CGM.SizeTy, CGM.SizeTy, KmpRoutineEntryPtrTy,
+                                CGM.SizeTy};
+    // Return void * and then cast to particular kmp_task_t type.
+    auto *FnTy =
+        llvm::FunctionType::get(CGM.VoidPtrTy, TypeParams, /*isVarArg=*/false);
+    RTLFn = CGM.CreateRuntimeFunction(FnTy, /*Name=*/"__kmpc_omp_target_task_alloc");
+    break;
+  }
   case OMPRTL__kmpc_omp_task: {
     // Build kmp_int32 __kmpc_omp_task(ident_t *, kmp_int32 gtid, kmp_task_t
     // *new_task);
     llvm::Type *TypeParams[] = {getIdentTyPointerTy(), CGM.Int32Ty,
                                 CGM.VoidPtrTy};
     auto *FnTy =
         llvm::FunctionType::get(CGM.Int32Ty, TypeParams, /*isVarArg=*/false);
     RTLFn = CGM.CreateRuntimeFunction(FnTy, /*Name=*/"__kmpc_omp_task");
CGOpenMPRuntime::emitTaskInit(CodeGenFunction &CGF, SourceLocation Loc,
   Address KmpTaskSharedsPtr = Address::invalid();
   if (!SharedsTy->getAsStructureType()->getDecl()->field_empty()) {
     KmpTaskSharedsPtr =
         Address(CGF.EmitLoadOfScalar(
                     CGF.EmitLValueForField(
                         TDBase, *std::next(KmpTaskTQTyRD->field_begin(),
                                            KmpTaskTShareds)),
                     Loc),
                 CGF.getNaturalTypeAlignment(SharedsTy));
     LValue Dest = CGF.MakeAddrLValue(KmpTaskSharedsPtr, SharedsTy);
     LValue Src = CGF.MakeAddrLValue(Shareds, SharedsTy);
     CGF.EmitAggregateCopy(Dest, Src, SharedsTy, AggValueSlot::DoesNotOverlap);
   }
   // Emit initial values for private copies (if any).
   TaskResultTy Result;
   if (!Privates.empty()) {
     emitPrivatesInit(CGF, D, KmpTaskSharedsPtr, Base, KmpTaskTWithPrivatesQTyRD,