[MLIR] Avoid adding debuginfo for a function if it contains calls that has no debug info.

[timshen]

Summary

Also add a verifier pass to ExecutionEngine.

It's hard to come up with a test case, since mlir-opt always add location info after parsing it (?)

[mehdi_amini]

mlir/lib/ExecutionEngine/OptUtils.cpp

116 ↗

(On Diff #293652)

This looks like an odd place to me: can we put this at the end of the translate when we emit LLVM IR?

[timshen]

Updated the place to verify the LLVM IR.

[timshen]

Adjust tests.

[mehdi_amini]

mlir/lib/ExecutionEngine/ExecutionEngine.cpp

240 ↗

(On Diff #293855)

Can't it be done in mlir::translateModuleToLLVMIR directly because of the FIXME above?

[timshen]

@mehdi_amini , can you take a look? River doesn't seem to be around atm.

[mehdi_amini]

In D88135#2293387, @timshen wrote:

@mehdi_amini , can you take a look? River doesn't seem to be around atm.

Did you miss my last question?
I'm fine with this right now, but I wanted to make sure I understand why we don't just verify inside translateModuleToLLVMIR directly, and if we could fix/change translateModuleToLLVMIR to have this?

[timshen]

Move the verification to translateLLVMIR.

[timshen]

In D88135#2293416, @mehdi_amini wrote:

In D88135#2293387, @timshen wrote:

@mehdi_amini , can you take a look? River doesn't seem to be around atm.

Did you miss my last question?
I'm fine with this right now, but I wanted to make sure I understand why we don't just verify inside translateModuleToLLVMIR directly, and if we could fix/change translateModuleToLLVMIR to have this?

I took "end of the translate" by "after the translate" not "within it but in the end", and moved it to ExecutionEngine where translateModuleToLLVMIR() was called. Moved to translateModuleToLLVMIR.

It does make more sense to move it to translateModuleToLLVMIR().

[mehdi_amini]

Awesome, thanks!

mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp

 
 #include "mlir/Target/LLVMIR.h"
 
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 #include "mlir/Translation.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/Support/ToolOutputFile.h"
 
 using namespace mlir;
 
 std::unique_ptr<llvm::Module>
 mlir::translateModuleToLLVMIR(ModuleOp m, llvm::LLVMContext &llvmContext,
                               StringRef name) {
-  return LLVM::ModuleTranslation::translateModule<>(m, llvmContext, name);
+  auto llvmModule =
+      LLVM::ModuleTranslation::translateModule<>(m, llvmContext, name);
+  if (verifyModule(*llvmModule))
+    emitError(m.getLoc(), "LLVM IR fails to verify");
+  return llvmModule;
 }
 
 namespace mlir {
 void registerToLLVMIRTranslation() {
   TranslateFromMLIRRegistration registration(
       "mlir-to-llvmir",
       [](ModuleOp module, raw_ostream &output) {
         llvm::LLVMContext llvmContext;

mlir/lib/Target/LLVMIR/DebugTranslation.cpp

 
 /// Translate the debug information for the given function.
 void DebugTranslation::translate(LLVMFuncOp func, llvm::Function &llvmFunc) {
   // If the function doesn't have location information, there is nothing to
   // translate.
   if (!compileUnit || !func.walk(interruptIfValidLocation).wasInterrupted())
     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().isa<UnknownLoc>() ? WalkResult::interrupt()
+                                                   : WalkResult::advance();
+          })
+          .wasInterrupted();
+  if (hasCallWithoutDebugInfo)
+    return;
+
   FileLineColLoc fileLoc = extractFileLoc(func.getLoc());
   auto *file = translateFile(fileLoc ? fileLoc.getFilename() : "<unknown>");
   unsigned line = fileLoc ? fileLoc.getLine() : 0;
 
   // TODO: This is the bare essentials for now. We will likely end
   // up with wrapper metadata around LLVMs metadata in the future, so this
   // doesn't need to be smart until then.
   llvm::DISubroutineType *type =

mlir/test/Target/llvmir-debug.mlir

 // RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
 
 // CHECK-LABEL: define void @func_no_debug()
 // CHECK-NOT: !dbg
 llvm.func @func_no_debug() {
   llvm.return loc(unknown)
 } loc(unknown)
 
 // CHECK-LABEL: define void @func_with_debug()
 // CHECK-SAME: !dbg ![[FUNC_LOC:[0-9]+]]
 llvm.func @func_with_debug() {
-  // CHECK: call void @func_no_debug()
-  // CHECK-NOT: !dbg
-  llvm.call @func_no_debug() : () -> () loc(unknown)
-
   // CHECK: call void @func_no_debug(), !dbg ![[CALLSITE_LOC:[0-9]+]]
   llvm.call @func_no_debug() : () -> () loc(callsite("mysource.cc":3:4 at "mysource.cc":5:6))
 
   // CHECK: call void @func_no_debug(), !dbg ![[FILE_LOC:[0-9]+]]
   llvm.call @func_no_debug() : () -> () loc("foo.mlir":1:2)
 
   // CHECK: call void @func_no_debug(), !dbg ![[NAMED_LOC:[0-9]+]]
   llvm.call @func_no_debug() : () -> () loc("named"("foo.mlir":10:10))