[Mlir-commits] [llvm] [mlir] Reland "[OMPIRBuilder] Don't use invalid debug loc in task proxy fn." (PR #218427)

[llvmlistbot at llvm.org]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-flang-openmp

Author: Abid Qadeer (abidh)

<details>
<summary>Changes</summary>

The proxy function built by `emitTargetTaskProxyFunction` inherited whatever debug location the builder happened to carry, which belongs to the target region rather than to the proxy. Save and restore the insertion point around the proxy and clear the debug location while it is emitted, so its instructions get none.

History of this change:

  Landed in https://github.com/llvm/llvm-project/pull/148284
  Reverted in https://github.com/llvm/llvm-project/pull/148728
  Sanitizer issue fixed in https://github.com/llvm/llvm-project/pull/148887

The revert was needed because the `InsertPointGuard` added here restores an insertion point that the `PostOutlineCB` callbacks had meanwhile invalidated: they erase the instruction the insertion point refers to, so the guard read freed memory in its destructor. #<!-- -->148887 clears the insertion point before those erases, which removes the dangling reference this patch would otherwise expose, so the change is safe to reapply now.

Verified by reverting #<!-- -->148887 on top of this patch and running the OpenMP translation tests under valgrind: omptarget-depend.mlir and omptarget-if-nowait.mlir report an invalid read in InsertPointGuard::~InsertPointGuard() of memory freed by Instruction::eraseFromParent() in createTask. With #<!-- -->148887 in place, all 185 tests are clean.

The test also needed updating for syntax that changed while this was out of tree: omp.target now takes a kernel_type clause, and omp.map.info no longer accepts name in the attribute dictionary.

---
Full diff: https://github.com/llvm/llvm-project/pull/218427.diff


2 Files Affected:

- (modified) llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp (+2) 
- (added) mlir/test/Target/LLVMIR/omptarget-debug-target-task.mlir (+49) 


``````````diff
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index 6f097fa205c34..1b04fc0b1c22a 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -9497,9 +9497,11 @@ static Function *emitTargetTaskProxyFunction(
 
   bool HasShareds = SharedArgsOperandNo > 0;
   bool HasOffloadingArrays = NumOffloadingArrays > 0;
+  IRBuilder<>::InsertPointGuard IPG(Builder);
   BasicBlock *EntryBB =
       BasicBlock::Create(Builder.getContext(), "entry", ProxyFn);
   Builder.SetInsertPoint(EntryBB);
+  Builder.SetCurrentDebugLocation(llvm::DebugLoc());
 
   SmallVector<Value *> KernelLaunchArgs;
   KernelLaunchArgs.reserve(StaleCI->arg_size());
diff --git a/mlir/test/Target/LLVMIR/omptarget-debug-target-task.mlir b/mlir/test/Target/LLVMIR/omptarget-debug-target-task.mlir
new file mode 100644
index 0000000000000..77adb4a712e1d
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/omptarget-debug-target-task.mlir
@@ -0,0 +1,49 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+module attributes {omp.is_target_device = false} {
+  llvm.func @omp_target_depend_() {
+    %0 = llvm.mlir.constant(39 : index) : i64
+    %1 = llvm.mlir.constant(1 : index) : i64
+    %2 = llvm.mlir.constant(40 : index) : i64
+    %3 = omp.map.bounds lower_bound(%1 : i64) upper_bound(%0 : i64) extent(%2 : i64) stride(%1 : i64) start_idx(%1 : i64)
+    %4 = llvm.mlir.addressof @_QFEa : !llvm.ptr
+    %5 = omp.map.info var_ptr(%4 : !llvm.ptr, !llvm.array<40 x i32>) map_clauses(from) capture(ByRef) bounds(%3) name("a") -> !llvm.ptr
+    omp.target kernel_type(generic) depend(taskdependin -> %4 : !llvm.ptr) map_entries(%5 -> %arg0 : !llvm.ptr) {
+      %6 = llvm.mlir.constant(100 : index) : i32
+      llvm.store %6, %arg0 : i32, !llvm.ptr
+      omp.terminator
+    } loc(#loc13)
+    llvm.return
+  } loc(#loc12)
+
+  llvm.mlir.global internal @_QFEa() {addr_space = 0 : i32} : !llvm.array<40 x i32> {
+    %0 = llvm.mlir.zero : !llvm.array<40 x i32>
+    llvm.return %0 : !llvm.array<40 x i32>
+  }
+}
+
+#loc1 = loc("test.f90":4:18)
+#loc2 = loc("test.f90":8:7)
+
+#di_file = #llvm.di_file<"test.f90" in "">
+#di_null_type = #llvm.di_null_type
+#di_compile_unit = #llvm.di_compile_unit<id = distinct[0]<>,
+ sourceLanguage = DW_LANG_Fortran95, file = #di_file, producer = "flang",
+ isOptimized = false, emissionKind = LineTablesOnly>
+#di_subroutine_type = #llvm.di_subroutine_type<
+  callingConvention = DW_CC_program, types = #di_null_type>
+#di_subprogram = #llvm.di_subprogram<id = distinct[1]<>,
+  compileUnit = #di_compile_unit, scope = #di_file, name = "main",
+  file = #di_file, subprogramFlags = "Definition|MainSubprogram",
+  type = #di_subroutine_type>
+#di_subprogram1 = #llvm.di_subprogram<compileUnit = #di_compile_unit,
+  name = "target", file = #di_file, subprogramFlags = "Definition",
+  type = #di_subroutine_type>
+
+
+#loc12 = loc(fused<#di_subprogram>[#loc1])
+#loc13 = loc(fused<#di_subprogram1>[#loc2])
+
+// CHECK: define internal void @.omp_target_task_proxy_func
+// CHECK-NOT: !dbg
+// CHECK: }

``````````

</details>


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