The way the LLVM dialect currently handles debuginfo is from an era before we had extensible locations. In LLVM, debuginfo locations have line/column and an attached scope. Because of the way that the location system worked when the dialect was created, the dialect uses mlir::FusedLoc<LLVM::DILocalScopeAttr>(loc) for all locations that have scopes attached.
Currently, we see a few main issues with this representation:
FusedLoc can mean many things based on the compiler you’re building. Especially in the ML space, fusion is a very common optimization. This can make it hard to tell exactly what you’re looking for at-a-glance.
If you want to represent more than just DILocalScopeAttr in that FusedLoc metadata (i.e. the name of the pass that did the fusion) you would end up breaking all the various utilities that currently rely on this implicit contract.
Higher level dialects/frontends construct debuginfo against the LLVM IR spec, and have structure built against that spec, and they have to throw it away to conform to this implicit FusedLoc contract; only for us to re-create the structure again when we translate the dialect to LLVM IR. This seems un-necessary if we expect the LLVM dialect to be a faithful representation of the LLVM dialect.
LoopAnnotationAttr stores its startLoc/endLoc as FusedLoc, carrying the debug scope as opaque metadata with no type-level guarantee that it is actually a DILocalScopeAttr. With the introduction of DILocationAttr, the import path will produce loop annotation locations as DILocationAttr directly, making the scope relationship explicit at the point of construction.
Current Representation
Debug scope is encoded via FusedLoc metadata in three patterns:
Function locations: FusedLocWith<DISubprogramAttr>({fileLoc})
At a high level, we propose adding a location attribute to the LLVM dialect, LLVM::DILocationAttr.
//===----------------------------------------------------------------------===//
// DILocationAttr
//===----------------------------------------------------------------------===//
def LLVM_DILocationAttr : LocationAttrDef<LLVM_Dialect, "DILocation"> {
let description = [{
Represents a debug location combining a source position and a debug info
scope for instruction-level and function-level debug locations.
Example:
``mlir
#llvm.di_location<loc("file.cpp":10:1) in #scope>
``
}];
let mnemonic = "di_location";
let parameters = (ins
"FileLineColLoc":$sourceLoc,
"DILocalScopeAttr":$scope
);
let assemblyFormat = "`<` $sourceLoc `in` $scope `>`";
let builders = [
AttrBuilderWithInferredContext<(ins
"FileLineColLoc":$sourceLoc, "DILocalScopeAttr":$scope
), [{
return $_get(sourceLoc.getContext(), sourceLoc, scope);
}]>,
];
}
Decisions Made
Use FileLineColLoc instead of just line/column and relying on the scope for the file
This is mainly for debugging purposes - the way MLIR prints locations and the way the diagnostic printer prints locations on diagnostics becomes quite unhelpful if you don’t have file/line/column inline on each op.
Use DILocalScopeAttr (not untyped Attribute) for the scope parameter
FusedLoc’s metadata is a bare Attribute with no type constraint. Producers can attach anything — the contract that holds a debug scope is purely by convention, checked only at consumption via runtime casts. DILocationAttr makes this a construction-time guarantee.
Continue to use CallSiteLoc instead of inlined_at.
Using CallSiteLoc doesn’t lose any representational capacity, and it avoids unnecessary additional infrastructure. It’s (subjectively) as easy to understand as inlined_at and similarly type-safe when you’re transforming the IR. Not to mention, CallSiteLoc is already supported by other MLIR utilities like the inliner, so by using it we don’t have to modify anything that would produce such a location.
LoopAnnotationAttr: startLoc/endLoc migration from FusedLoc to LocationAttr
The parameter type will be LocationAttr rather than DILocationAttr because DILocationAttr does not carry an inlinedAt field, relying instead on MLIR’s CallSiteLoc to represent inlining chains (see above for reasoning to stick with CallSiteLoc). When the inliner processes a loop annotation, it wraps startLoc/endLoc in CallSiteLoc to preserve the call-site chain, matching how LLVM IR represents inlined loop metadata via the inlinedAt field on !DILocation. LocationAttr accommodates both the pre-inlining representation (DILocationAttr) and the post-inlining representation (CallSiteLoc wrapping DILocationAttr), with DebugTranslation::translateLoc handling both transparently on export.
Transition Plan
Add DILocationAttr alongside FusedLoc
Update all in-tree producers/consumers to use DILocationAttr
Remove support for FusedLoc
Initial implementation
Currently contains only basic implementation of DiLocation. The plan to add followup commits to it to demonstrate the full scope, and then follow up with individual PRs for each commit.
In general, aligning LLVM dialect with what LLVM IR does is the right direction.
I don’t quite understand what this means. We still have extensible locations. The example below literally introduces a new location kind from what I can see.
Does this mean we have a file listed twice: in FileLineColLoc and in the scope? This feels dangerous.
The FusedLoc version of attaching scopes to FileLineColLoc came from before dialects could define their own location attributes
Yep, and agreed. The trade-off is that if you don’t duplicate the filename, MLIR’s diagnostic printer doesn’t know how to find the scope (and from it, the file) to report the error, so you end up with much more opaque errors. That could be a further improvement to the diagnostic printer though!
Ultimately the files in FileLineColLoc inside DILocationAttr just get ignored in favor of the file in the scope - in the past (downstream) I’ve implemented a verifier that asserts the file in the scope and the file in FileLineColLoc must match, but that was somewhat problematic in cases where the scope is a reference; e.g. when you de-duplicate subprograms or something because the attribute verifier can’t look at the rest of the IR to resolve the reference.
I support the change since the new representation is closer to LLVM IR’s debug info representation.
That’s a good point. I wonder if we could have a `FileLineColLocInterface` which is implemented by both `FileLineColLoc` and `DILocationAttr`? That way we could presumably avoid the duplication and have existing infra such as the diagnostics printer use the interface rather than `FileLineColLoc` directly.
Is it possible to make the file field NULL instead and have a verifier for that from DILocationAttr? Or is this the case where the diagnostic printer gets confused?
Also +1 to @gysit’s idea of an interface. Let’s at least try scoping the change to see how hard it would be to implement.
I think this FileLineColInterface idea could work here - that would allow us to simply avoid having the FileLineColLoc into DILocationAttr, we’d just store the line/col ranges instead. I suspect that’d be a pretty big change though; I’m on vacation this week so I’ll do some scoping when I get back (or @ayermolo if you want to take a look?). I’m generally +1 on making the builtin locations less ‘special’ so this seems like a step in the right direction to me. @gysit@ftynse how would y’all feel about that being a step on the transition plan, or do you see it as a prerequisite for landing the change at all? My thinking is that since it’s largely in core infra we could stage it as an infra change + a small change to adopt it in the LLVM dialect on top of an attribute that already exists.
The case where the diagnostic printer gets confused is more interesting - consider the case where the scope for the location is a subprogram, but you don’t want to store the DISubprogramAttr on every op, so you store a ref instead (SymbolRefAttr, or a name or something) and store the DISubprogramAttr elsewhere. In that case, you don’t have direct access to the attribute that has the actual file, so unless you duplicate it you just don’t have access.
I would hope the change is not too large by itself.
I am fine if it is done in a separate step but it may cause some extra work since the new `DILocationAttr` needs to be updated shortly after landing. I would probably try to prototype the interface first as suggested and then decide if splitting makes sense.
The other way is to enable locations to have hook as to how they want to emit a diagnostic. One way is for file line col, call stack, etc to all be interfaces too, the other is a way to interface with diagnostic emission.
I think changing core infra should be separate from changing LLVM dialect usage of it based on a very simple principle: someone skimming through commit messages / changelog is more likely to notice a dedicated core infra change and adopt it in their usage. Otherwise, as long as we have confidence that both can land, or chose a completely different direction, I’m fine with any order.
Admittedly, the exact attribute structure is not in my short-term memory and this is tangential, but why can’t we store a DISubprogramAttr on every op. Attributes are owned by the context, it’s not more expensive to store a DISubprogramAttr than it is to store a SymbolRefAttr, both are pointers. It is actually less expensive to use the former because there’s would be no indirection via a symbol table. Printing/parsing gets more complicated though.
Thanks! I was thinking this but couldn’t quite verbalize: can we have some sort of DiagnosticEmissionAttrInterface? So that the emitter calls that instead of assuming file/line/col. I had to write “stack unwinding” logic based on locations a couple of times so it would be appreciated to just have a default mechanism. That being said, this may be a larger design change so I’m happy to spitball but won’t insist on this being implemented as a precondition.
I was wondering about this, making it a default member on location that one has to implement (maybe we could do a virtual default or some such) and then basically we can remove that existing logic inside the emitter based on built-in type and move it out (default emission then just string without location). I think it may not be terribly large, but yes not directly related here.
That’s true - except for when you might want to change something. Then, you have to walk every op that might have that DISubprogramAttr on it and update its location (so, every op in the function). This doesn’t matter for the LLVM dialect, because you do store a DISubprogramAttr on every op, but for downstream dialects that need to model something similar, they run into this problem. I’ve personally run into it twice in the last year The interface (or something similar) would solve the problem in the LLVM dialect; each location has everything it needs locally. As long as we don’t have to change/update debug info too much it’s not a big deal. In my experience/use cases/people I talk to LLVM tends to be an ‘exit’ dialect (would be curious if it’s not for some folks) so I’m not anticipating needing huge debug info changes once the program lands in the LLVM dialect.
Yeah sorry - I should have clarified. Agreed - we’ll scope the infra change as a pure infra change and then we can do DILocationAttr on top of that.
I do think this is an interesting idea, but agreed that I don’t think it needs to happen before DILocationAttr can land. I would be willing to take a look afterwards though
@bzcheeseman inquiring minds are dying to know: do you plan to provide a versioned bytecode serialization format/representation for this new attribute?
For DILocationAttr? As much as the other LLVM dialect debuginfo attributes, but I wasn’t planning anything extra on top of that. Any particular reason?
Okay, I see why you’d want the indirection. Are you walking symbol uses or just updating the symbol definition? One could also consider adding mutable attributes for this purpose, we already have mutable types and since they share the implementation, it is pretty straightforward. This would give you an attribute with a fixed name and a varying “impl” field that can point to a further attribute without leaving the attribute hierarchy.
OK - I actually think this could be a pretty simple change. If we introduce the interface and then basically just do this:
diff --git a/mlir/lib/IR/Diagnostics.cpp b/mlir/lib/IR/Diagnostics.cpp
index f4c9242ed347..7b308a0d5b36 100644
--- a/mlir/lib/IR/Diagnostics.cpp
+++ b/mlir/lib/IR/Diagnostics.cpp
@@ -263,6 +263,8 @@ void DiagnosticEngineImpl::emit(Diagnostic &&diag) {
return;
auto &os = llvm::errs();
+ if (auto flcLoc = llvm::dyn_cast<FileLineColLocInterface>(diag.getLocation()))
+ os << flcLoc.resolveAsFileLineCol() << ": ";
if (!llvm::isa<UnknownLoc>(diag.getLocation()))
os << diag.getLocation() << ": ";
os << "error: ";
The main issue is that custom diagnostic engines will have to copy this implementation, but IMO that’s fine. The other option would be to have the diagnostic have a method or something that could call this, but I think I’d rather keep Diagnostic generic to hold any Location and have the emitter handle the casting. Thoughts @gysit@ftynse@ayermolo?
Mostly just updating the symbol definition. The mutable attribute thing is an interesting idea - admittedly I haven’t looked into how mutable types work so I haven’t thought it through completely. If you have ideas on what I should look at for this separate use case that’d be awesome
I am not a big fan of mutable attributes. They are not well supported by the attribute infrastructure (no tablegen support - AFAIK - and the uniquing then works only on the non mutable part) and thus require more manual work and care when using them.
