[flang-commits] [flang] [flang][debug] Generate DISubprogramAttr for omp::TargetOp. (PR #138039)

[Sergio Afonso via flang-commits]

================
@@ -510,8 +542,73 @@ void AddDebugInfoPass::handleFuncOp(mlir::func::FuncOp funcOp,
       subTypeAttr, entities, /*annotations=*/{});
   funcOp->setLoc(builder.getFusedLoc({l}, spAttr));
 
+  /* When we process the DeclareOp inside the OpenMP target region, all the
+     variables get the DISubprogram of the parent function of the target op as
+     the scope. In the codegen (to llvm ir), OpenMP target op results in the
+     creation of a separate function. As the variables in the debug info have
+     the DISubprogram of the parent function as the scope, the variables
+     need to be updated at codegen time to avoid verification failures.
+
+     This updating after the fact becomes more and more difficult when types
+     are dependent on local variables like in the case of variable size arrays
+     or string. We not only have to generate new variables but also new types.
+     We can avoid this problem by generating a DISubprogramAttr here for the
+     target op and make sure that all the variables inside the target region
+     get the correct scope in the first place. */
+  funcOp.walk([&](mlir::omp::TargetOp targetOp) {
+    unsigned line = getLineFromLoc(targetOp.getLoc());
+    mlir::StringAttr Name =
+        getTargetFunctionName(context, targetOp.getLoc(), funcOp.getName());
+    mlir::LLVM::DISubprogramFlags flags =
+        mlir::LLVM::DISubprogramFlags::Definition |
+        mlir::LLVM::DISubprogramFlags::LocalToUnit;
+    if (isOptimized)
+      flags = flags | mlir::LLVM::DISubprogramFlags::Optimized;
+
+    mlir::DistinctAttr recId =
+        mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+    mlir::DistinctAttr Id =
+        mlir::DistinctAttr::create(mlir::UnitAttr::get(context));
+    llvm::SmallVector<mlir::LLVM::DITypeAttr> types;
+    types.push_back(mlir::LLVM::DINullTypeAttr::get(context));
----------------
skatrak wrote:

In this approach, the argument types correspond to the entry block arguments of the `omp.target` operation, and my understanding is that this list of types is intended to correspond to the argument list of the outlined function later generated for that target region. Let me know if I'm wrong about that.

If that is the case, I believe that this wouldn't be the correct set of types. If we look at `createOutlinedFunction` in the OMPIRBuilder, we can see that for the target device an extra pointer argument is added (which here we aren't doing here, but instead fixing it up at the end of that codegen function) and the rest of the argument list is passed by the caller. Tracking down callers, this eventually corresponds to the `Inputs` arguments for `OpenMPIRBuilder::createTarget`, which in OpenMP to LLVM IR translation corresponds to the `kernelInput` vector in `convertOmpTarget`. Looking at how it's initialized it's clear that the mapping between `omp.target` op and its associated outlined LLVM IR function is not so straightforward.

Perhaps the easiest way to deal with this would be to use some placeholder type early on and then update it during codegen, when we have access to the full list of arguments. This hopefully seems like it should not be an issue, since that's what's already done for the extra argument added for the device. Otherwise, we'd have to somehow replicate part of the map-related logic used to populate the argument list here.

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