[Mlir-commits] [mlir] a03ea35 - [mlir][LLVM] Introduce verfier for call debug locations

[Christian Ulmann]

Author: Christian Ulmann
Date: 2023-08-07T05:53:27Z
New Revision: a03ea35e59e4b9a678a714ba5c6639e9667f5854

URL: https://github.com/llvm/llvm-project/commit/a03ea35e59e4b9a678a714ba5c6639e9667f5854
DIFF: https://github.com/llvm/llvm-project/commit/a03ea35e59e4b9a678a714ba5c6639e9667f5854.diff

LOG: [mlir][LLVM] Introduce verfier for call debug locations

This commit introduces a debug location verifier for the LLVM dialect's
call operation. LLVM does not allow calls to have no debug location when
they reference an inlinable function with debug information. This
apparenlty breaks assumptions of LLVM's inliner.

So far, there was a hack in the LLVM export that avoided this case to
be triggered, but that hack causes issues when debug intrinsics are
involved. Link to the revision that inroduced the export hack:
https://reviews.llvm.org/D88135

LLVM's verifier as a reference: https://github.com/llvm/llvm-project/blob/2df05cd01c17f3ef720e554dc7cde43df27e5224/llvm/lib/IR/Verifier.cpp#L3546

Reviewed By: gysit, zero9178

Differential Revision: https://reviews.llvm.org/D157096

Added: 
    mlir/test/Dialect/LLVMIR/invalid-call-location.mlir

Modified: 
    mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
    mlir/lib/Target/LLVMIR/DebugTranslation.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index 2d04aab307d748..2cb4cace5e445b 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -1008,6 +1008,31 @@ MutableOperandRange CallOp::getArgOperandsMutable() {
                              getCalleeOperands().size());
 }
 
+/// Verify that an inlinable callsite of a debug-info-bearing function in a
+/// debug-info-bearing function has a debug location attached to it. This
+/// mirrors an LLVM IR verifier.
+static LogicalResult verifyCallOpDebugInfo(CallOp callOp, LLVMFuncOp callee) {
+  if (callee.isDeclaration())
+    return success();
+  auto parentFunc = callOp->getParentOfType<FunctionOpInterface>();
+  if (!parentFunc)
+    return success();
+
+  auto hasSubprogram = [](Operation *op) {
+    return op->getLoc()
+               ->findInstanceOf<FusedLocWith<LLVM::DISubprogramAttr>>() !=
+           nullptr;
+  };
+  if (!hasSubprogram(parentFunc) || !hasSubprogram(callee))
+    return success();
+  bool containsLoc = !isa<UnknownLoc>(callOp->getLoc());
+  if (!containsLoc)
+    return callOp.emitError()
+           << "inlinable function call in a function with a DISubprogram "
+              "location must have a debug location";
+  return success();
+}
+
 LogicalResult CallOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
   if (getNumResults() > 1)
     return emitOpError("must have 0 or 1 result");
@@ -1046,6 +1071,8 @@ LogicalResult CallOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
       return emitOpError() << "'" << calleeName.getValue()
                            << "' does not reference a valid LLVM function";
 
+    if (failed(verifyCallOpDebugInfo(*this, fn)))
+      return failure();
     fnType = fn.getFunctionType();
   }
 

diff  --git a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
index ca5dde7a9376e2..fcd0ad3676952f 100644
--- a/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
+++ b/mlir/lib/Target/LLVMIR/DebugTranslation.cpp
@@ -62,20 +62,6 @@ void DebugTranslation::translate(LLVMFuncOp func, llvm::Function &llvmFunc) {
   if (!debugEmissionIsEnabled)
     return;
 
-  // If we are to create debug info for the function, we need to ensure that all
-  // inlinable calls in it are with debug info, otherwise the LLVM verifier will
-  // complain. For now, be more restricted and treat all calls as inlinable.
-  const bool hasCallWithoutDebugInfo =
-      func.walk([&](LLVM::CallOp call) {
-            return call.getLoc()->walk([](Location l) {
-              return isa<UnknownLoc>(l) ? WalkResult::interrupt()
-                                        : WalkResult::advance();
-            });
-          })
-          .wasInterrupted();
-  if (hasCallWithoutDebugInfo)
-    return;
-
   // Look for a sub program attached to the function.
   auto spLoc =
       func.getLoc()->findInstanceOf<FusedLocWith<LLVM::DISubprogramAttr>>();

diff  --git a/mlir/test/Dialect/LLVMIR/invalid-call-location.mlir b/mlir/test/Dialect/LLVMIR/invalid-call-location.mlir
new file mode 100644
index 00000000000000..ff819b65681281
--- /dev/null
+++ b/mlir/test/Dialect/LLVMIR/invalid-call-location.mlir
@@ -0,0 +1,35 @@
+// RUN: not mlir-opt %s -split-input-file 2>&1 | FileCheck %s
+
+// This test is in a separate file because the location tracking of verify
+// diagnostics does not work for unknown locations.
+
+#di_file = #llvm.di_file<"file.cpp" in "/folder/">
+#di_compile_unit = #llvm.di_compile_unit<
+  sourceLanguage = DW_LANG_C_plus_plus_14, file = #di_file,
+  isOptimized = true, emissionKind = Full
+>
+#di_subprogram = #llvm.di_subprogram<
+  compileUnit = #di_compile_unit, scope = #di_file,
+  name = "missing_debug_loc", file = #di_file,
+  subprogramFlags = "Definition|Optimized"
+>
+#di_subprogram1 = #llvm.di_subprogram<
+  compileUnit = #di_compile_unit, scope = #di_file,
+  name = "invalid_call_debug_locs", file = #di_file,
+  subprogramFlags = "Definition|Optimized"
+>
+#loc = loc(unknown)
+#loc1 = loc("file.cpp":24:0)
+#loc2 = loc(fused<#di_subprogram>[#loc1])
+#loc3 = loc("file.cpp":42:0)
+#loc4 = loc(fused<#di_subprogram1>[#loc3])
+
+llvm.func @missing_debug_loc() {
+  llvm.return
+} loc(#loc2)
+
+llvm.func @invalid_call_debug_locs() {
+// CHECK: <unknown>:0: error: inlinable function call in a function with a DISubprogram location must have a debug location
+  llvm.call @missing_debug_loc() : () -> () loc(#loc)
+  llvm.return
+} loc(#loc4)