Potential bug in DebugTranslation::translate

MLIR
1

I’m looking at a failure that looks like this:
inlinable function call in a function with debug info must have a !dbg location

After investigating i found this code in DebugTranslation::translate

// 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();

I believe that this code isn’t sufficient to accomplish its task. The problem, in my case, is that i have a NameLoc on a call and the debug tag translation code will use its child tag when mapping, and the child tag is an UnknownLoc.

I’m using this workaround for now, but it doesn’t feel very robust.

func.walk([&](LLVM::CallOp call) {                                                     
      auto loc = call.getLoc();                                                        
      if (loc.isa<UnknownLoc>()) {                                                     
        return WalkResult::interrupt();                                                
      } else if (auto nameLoc = loc.dyn_cast<NameLoc>()) {                             
        if (nameLoc.getChildLoc().isa<UnknownLoc>())                                   
          return WalkResult::interrupt();                                              
      } else {                                                                         
        // TODO: other location types that could result in UnknownLoc                  
        // during translation                                                          
      }                                                                                
      return WalkResult::advance();                                                    
    })

I’m looking to get some confirmation that i’ve correctly identified the problem and some guidance on a better way to fix this before pushing a PR.

2

How does the code that figure out the location to use latter work? I would refactor it and make sure we have a consistent handling.

3

That’s a great point, i originally tried to directly leverage that code, which is in DebugTranslation::translateLoc but i’m not very faimilar with this code. I ran into trouble as it needs a DIScope which we don’t create until later.

I’m looking over the code again as i write this, and i’m wondering if LocationAttr::walk is what i want with a functor that checks for unknown locations.

4

Something like this i think:


  // 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 l.isa<UnknownLoc>() ? WalkResult::interrupt()
                                         : WalkResult::advance();
            });
          })
          .wasInterrupted();
  if (hasCallWithoutDebugInfo)
    return;
5

Diff is up now:

:gear: D121633 [MLIR] UnknownLoc on Inlinable Calls in LLVMIR Translation](⚙ D121633 [MLIR] UnknownLoc on Inlinable Calls in LLVMIR Translation)