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

[Dinistro]

Summary

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

[zero9178]

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1028

I believe it should be suffiecent to check !isa<UnknownLoc>(callOp->getLoc()) here as UnknownLoc can't occur in fused locs.
The constructor of FusedLoc sorts them out of the list: https://github.com/llvm/llvm-project/blob/46b2ad0224d3c9a9cc299211213e2cf677f5a78c/mlir/lib/IR/Location.cpp#L104

Should the variable also maybe be called just containsLoc? containsLLVMDebugLoc sounds to me like its talking about actuall LLVM IR past the exporter, while we are dealing with MLIR locs here

[zero9178]

LGTM otherwise

[Dinistro]

address review comments

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1028

I was not aware that UnknownLocs are filtered out, thanks for pointing it out.

[gysit]

LGTM

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

1013

ultra nit:

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp

Operation::operand_range CallOp::getArgOperands() {
   return getOperands().drop_front(getCallee().has_value() ? 0 : 1);
 }
 
 MutableOperandRange CallOp::getArgOperandsMutable() {
   return MutableOperandRange(*this, getCallee().has_value() ? 0 : 1,
                              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");
 
   // Type for the callee, we'll get it differently depending if it is a direct
   // or indirect call.
   Type fnType;
 
if (!callee)
       return emitOpError()
              << "'" << calleeName.getValue()
              << "' does not reference a symbol in the current scope";
     auto fn = dyn_cast<LLVMFuncOp>(callee);
     if (!fn)
       return emitOpError() << "'" << calleeName.getValue()
                            << "' does not reference a valid LLVM function";
 
+    if (failed(verifyCallOpDebugInfo(*this, fn)))
+      return failure();
     fnType = fn.getFunctionType();
   }
 
   LLVMFunctionType funcType = llvm::dyn_cast<LLVMFunctionType>(fnType);
   if (!funcType)
     return emitOpError("callee does not have a functional type: ") << fnType;
 
   // Indirect variadic function calls are not supported since the translation to

mlir/lib/Target/LLVMIR/DebugTranslation.cpp

 /// Finalize the translation of debug information.
 void DebugTranslation::finalize() {}
 
 /// Translate the debug information for the given function.
 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>>();
   if (!spLoc)
     return;
   llvmFunc.setSubprogram(translate(spLoc.getMetadata()));
 }
 

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

+// 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)