[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

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/ExecutionEngine/ExecutionEngine.cpp

 #include "llvm/ExecutionEngine/Orc/CompileUtils.h"
 #include "llvm/ExecutionEngine/Orc/ExecutionUtils.h"
 #include "llvm/ExecutionEngine/Orc/IRCompileLayer.h"
 #include "llvm/ExecutionEngine/Orc/IRTransformLayer.h"
 #include "llvm/ExecutionEngine/Orc/JITTargetMachineBuilder.h"
 #include "llvm/ExecutionEngine/Orc/RTDyldObjectLinkingLayer.h"
 #include "llvm/ExecutionEngine/SectionMemoryManager.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/Verifier.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/ToolOutputFile.h"
 
 #define DEBUG_TYPE "execution-engine"
Expected<std::unique_ptr<ExecutionEngine>> ExecutionEngine::create(
   // FIXME: the triple should be passed to the translation or dialect conversion
   // instead of this.  Currently, the LLVM module created above has no triple
   // associated with it.
   setupTargetTriple(llvmModule.get());
   packFunctionArguments(llvmModule.get());
 
   auto dataLayout = llvmModule->getDataLayout();
 
+  if (verifyModule(*llvmModule))
+    return make_string_error("LLVM IR fails to verify");
+
   // Callback to create the object layer with symbol resolution to current
   // process and dynamically linked libraries.
   auto objectLinkingLayerCreator = [&](ExecutionSession &session,
                                        const Triple &TT) {
     auto objectLayer = std::make_unique<RTDyldObjectLinkingLayer>(
         session, []() { return std::make_unique<SectionMemoryManager>(); });
 
     // Register JIT event listeners if they are enabled.

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