[Mlir-commits] [clang] [llvm] [mlir] [OMPIRBuilder] - Handle dependencies in `createTarget` (PR #93977)

[Sergio Afonso]

================
@@ -5272,36 +5682,53 @@ static void emitTargetCall(OpenMPIRBuilder &OMPBuilder, IRBuilderBase &Builder,
   Value *DynCGGroupMem = Builder.getInt32(0);
 
   bool HasNoWait = false;
+  bool HasDependencies = Dependencies.size() > 0;
+  bool RequiresOuterTargetTask = HasNoWait || HasDependencies;
 
   OpenMPIRBuilder::TargetKernelArgs KArgs(NumTargetItems, RTArgs, NumIterations,
                                           NumTeamsVal, NumThreadsVal,
                                           DynCGGroupMem, HasNoWait);
 
-  Builder.restoreIP(OMPBuilder.emitKernelLaunch(
-      Builder, OutlinedFn, OutlinedFnID, EmitTargetCallFallbackCB, KArgs,
-      DeviceID, RTLoc, AllocaIP));
+  // The presence of certain clauses on the target directive require the
+  // explicit generation of the target task.
+  if (RequiresOuterTargetTask) {
+    OMPBuilder.emitTargetTask(OutlinedFn, OutlinedFnID,
----------------
skatrak wrote:

I agree with you, since I have noticed this redundant pattern as well, though in a way I think it makes sense from a uniformity point of view. There are several codegen functions that return an insertion point, which should be used by the caller to proceed with any further codegen.

I think that the fact that oftentimes the insertion point that's being returned is indeed already the current one shouldn't be relied on by the caller unless we make this a contract followed by all such functions. To be consistent, I think these codegen function must either always return an insertion point which must be used by the caller (the current approach) or all codegen functions must not return an insertion point and set it themselves to the spot follow-up codegen should go.

In my opinion, I believe it's currently better to keep the current approach even though it results in some redundant setting of the insertion point. Skipping the call to `Builder.restoreIP()` in the caller could result in unintended problems in the future, since the current contract is that the returned insertion point is the one that should be used and not the current one.

https://github.com/llvm/llvm-project/pull/93977