[RFC][MLIR] LLVM DILocationAttr

Looks fine in principle. I expect the majority of the users to install a custom diagnostic hanlder, not change the engine wholesale.

So a mutable type has an immutable key and a mutable “body”, both stored in the context. It looks like you can directly transpose what you currently do with symbols in IR onto this: the current symbol name becomes the key, and whatever was in the symbol definition becomes the body. If you need to mass-change the body, you can do so by constructing a new instance of the attribute give the unique key and just mutate it. You can look at LLVMStructType as an example: llvm-project/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.td at 598f3535fa96ee599d670ede3840beeca570ec1f · llvm/llvm-project · GitHub.

… because nobody added that, there’s like one use case upstream so little need to increase complexity of the infra. The uniquing working on the non-mutable part is a feature, I don’t see how we would unique the mutable part, or why.

Yes that is unavoidable I think.

I meant more that the consequences of that may be unexpected for example when comparing attribute pointers (which sometimes is fine, for example, in case of LLVMStructType). I don’t understand the problem discussed above well enough to judge if a mutable attribute makes sense. Just wanted to mention that the feature is tricky to use in my experience.

Ok, synced with Aman offline, so to summarize discussion/sanity check my understanding,
and what should be done.

The initial DILocationAttr proposal stores a FileLineColLoc (file + line +
col) alongside a DILocalScopeAttr. The scope contains a DIFileAttr which
also carries the filename. So the file identity is stored twice with nothing
enforcing they match.

This duplication is not new — the existing FusedLoc representation has the
same issue. FusedLocWith<DISubprogramAttr>({FileLineColLoc(...)}, subprogram)
carries the filename in the FileLineColLoc child and again in the
subprogram’s DIFileAttr. Every consumer extracts the filename from
FileLineColLoc and ignores what’s in the scope. Introducing DILocationAttr
gives us a chance to clean this up.

The solution is to have DILocationAttr store only line + column + scope
(no FileLineColLoc) and derive the filename from the scope’s DIFileAttr.
This mirrors LLVM IR’s DILocation, which stores line, column, and scope, and
gets the filename via getScope()->getFilename().

The problem is that MLIR infrastructure is
hardcoded to FileLineColLoc — it uses findInstanceOf<FileLineColLoc>() and
dyn_cast<FileLineColLoc>() to extract file/line/column. A DILocationAttr
that doesn’t contain a FileLineColLoc would be invisible to all of this.

The fix is a FileLineColRangeInterface that both FileLineColRange and
DILocationAttr implement. Consumers switch from the concrete type to the
interface, and any location that provides file/line/column works automatically.
On LLVM level DILocation doesn’t have a concept of ranges, but Aman mentioned
Clang does. So it doesn’t hurt to keep it generic/future proof.

Back of the napkin implementation.

Define an AttrInterface in BuiltinAttributeInterfaces.td that any location
attribute can implement to say “I carry file/line/column info”:

def FileLineColRangeInterface : AttrInterface<"FileLineColRangeInterface"> {
  let description = [{
    Interface for location attributes that carry file, line, and column info.
  }];
  let cppNamespace = "::mlir";
  let methods = [
    InterfaceMethod<"Get the filename", "::mlir::StringAttr", "getFilename">,
    InterfaceMethod<"Get the start line", "unsigned", "getStartLine">,
    InterfaceMethod<"Get the start column", "unsigned", "getStartColumn">,
    InterfaceMethod<"Get the end line", "unsigned", "getEndLine">,
    InterfaceMethod<"Get the end column", "unsigned", "getEndColumn">,
  ];
  let extraClassDeclaration = [{
    unsigned getLine() { return getStartLine(); }
    unsigned getColumn() { return getStartColumn(); }
  }];
}

Have FileLineColRange implement it — trivial since it already has all these
methods. FileLineColLoc inherits it for free.

Then update the handful of places in MLIR that currently hardcode
FileLineColLoc to use the interface type instead. Behavior stays identical — the only
implementor at this stage is FileLineColRange. The point is that the system is
now open, so when DILocationAttr implements the interface in a follow-up PR,
all these consumers pick it up automatically with zero additional changes.

How DILocationAttr will look after this

With the interface in place, DILocationAttr drops FileLineColLoc and stores
only what LLVM IR’s DILocation stores — line, column, and scope:

def LLVM_DILocationAttr : LocationAttrDef<LLVM_Dialect, "DILocation",
    [DeclareAttrInterfaceMethods<FileLineColRangeInterface>]> {
  let mnemonic = "di_location";
  let parameters = (ins
    "unsigned":$line,
    "unsigned":$column,
    "DILocalScopeAttr":$scope
  );
  let assemblyFormat = "`<` $line `:` $column `in` $scope `>`";
}

The interface methods are implemented by:

  • getLine() / getColumn() — return the stored fields directly
  • getFilename() — delegate to the scope: getScope().getFile().getName(),
    same as LLVM’s DILocation::getFilename() calls getScope()->getFilename()
  • getStartLine() / getEndLine() — both return getLine() (it’s a point)
  • getStartColumn() / getEndColumn() — both return getColumn()

Result no file duplication. The filename lives only in the scope’s DIFileAttr.
Diagnostics, breakpoints, and other infra see DILocationAttr through the
interface and extract file/line/column without knowing or caring about the
concrete type.

LLVM_DIFileAttr stores the filename as two separate attributes, so this could only work by creating a new attribute when queried (e.g., get would be mutating and internalize a new Attribute - if we were willing to do that, then we could also just enable casting to FileLineColLoc from LLVM DILocation by materializing a new FileLineColLoc attribute, that feels wasteful though). Which is also true if one used StringRef, so the way DIFileAttr is defined would result in materialization (e.g., a std::string would need to be created at a minimum).

Alternative: just make DIFileAttr a FileLineColLoc “derived” [1] class, making it return the directory and filename using StringRef operations (or caching the split point if ends up truly expensive, but this is a rfind from back). Then the duplication goes away, it is cheap to get for error emissions, more interning than today (currently it would be 3 strings interned vs 1), no interface needed, while it is still structured/C++ typed accessors.

[1]: Well its really just an Attribute that immediately wraps FileLineColLoc than derived. All the accessors just point through, but conceptually. And similarly LLVM_DILocationAttr just contains DILocalScopeAttr now as that has nested inside FileLineColRangeLoc of it and so all the error emissions etc just works.

Is that a problem? I wouldn’t imagine this happens a lot in ‘normal’ compilations so I’m not super concerned about the cost of that.

That’s more or less what the original proposal was, though done through ODS rather than C++, unless I’m misunderstanding your suggestion? DIFileAttr doesn’t have line/col information and it’s attached to scopes, so I substituted DILocationAttr for DIFileAttr in just make DIFileAttr a FileLineColLoc “derived” [1] class. Please do correct my understanding if I’m wrong!

Not sure the interface goes away in this case? I think I’m maybe still just not understanding what you’re proposing here.

IMO the interface makes sense so that other dialects can get the benefits of error emission the same way FileLineColLoc has. However, I’m not convinced that the cost of assembling the filename is high enough to warrant optimizing around that use case. I see 2 main situations where we would assemble a filename: (1) to produce an error, which is an exceptional case and I’m not super worried about a little extra string munging with all the string munging/assembly that happens for diagnostics already, and (2) to convert a bunch of locations to another dialect - in which case I’d expect the user to cache the StringAttr after creating it once if that level of optimization is a concern of theirs.

It’s wasteful and interned until end of context. We already have a way for just specifying a filename and there is no need really to do a hash lookup, grab a lock and add a new item here (especially if one does parallel compilation and every remark isn’t a fatal error) just to query what should be cheap. So it’s doing more work and incurring more memory than the alternative. Yes it may only trigger for error cases, when debugging (e.g., you now mutate the context while doing a greedy pattern rewrite debugging) and source maps generation, but there isn’t clear upside.

Your original proposal was creating a new location attribute that contained a FileLineColLoc and a DILocalScopeAttr. I’m suggesting changing the DIFileAttr that is nested inside of DILocalScope to capture the filename using standard filename location attribute (could be singular location or location and split index - the latter giving same performance and cost as today), all is still defined in ODS but it only contains a DILocalScopeAttr as member. DiFileAttr interface doesn’t need to change, it’s still at most two attributes, the querying & conversion cost could be same.

Interface adds a cost (querying if an attribute is a specific kind vs querying if it implements and interface). It’s effectivel inheritance vs composition level here. By composing you are getting the benefit and functionality already.

Sure, but (a) the hash should be cheap, and (b) because it’s interned until end of context it would get re-used (and not re-allocated), right? Filenames/paths are unlikely to change over the course of a single compilation. That said, I don’t disagree that it does take more work and incur more memory allocation, so let’s dig in.

What about the other way around, and having FileLineColLoc refer to a FileAttr like DIFileAttr does? Then we could standardize on directory + file everywhere in the same way that most debugging tools do? Alternatively, the interface could standardize on directory + filename as StringRef, which would be the smallest overall change - it would avoid having to construct any strings for any of the cases. FileLineColLoc returns directory as an rfind, filename as the last segment, then DIFileAttr just returns the appropriate StringRefs.

Having DIFileAttr contain a FileLineColLoc feels really off to me because they’re different things at different layers. Plus, having DIFileAttr contain any kind of line/col information even hidden feels like a recipe for confusion. I also don’t think that would solve the problem the interface is trying to solve - the DILocationAttr would hold the line/col information for the op, so line/col on the DIFileAttr would be 0, so we’d still need some way to resolve it all together, hence the interface still not going away.

Yeah I get that, function call + unsigned int equality vs function call + log(N) lookup in a list, but I’m still somewhat skeptical of this argument mostly just because I don’t have concrete data/experience that inheritance vs composition causes significant slowdowns in compile time in this case.

Umm, no particular opinion about this, beyond that on DIFileAttr we should be able to reconstruct original director and file name. So just doing rfind probably won’t work.

@jpienaar WDYT about Amans proposal?

Sorry on vacation, so takes a bit of time in between.

Good point, LLVM Language Reference Manual — LLVM 23.0.0git documentation does talk about that, so one has to retain the splitting point to roundtrip.

FileLineColLoc is optimized and takes no more space in memory than StringAttr unless needed (and even then it is pure ints post, not attributes). DIFileAttr is equivalent to using 2 FileLineColRange’s space. While 90% of the time you want the full filename, that’s what all our current output is, SourgeMgr’s getBufferIdentifier() also reports full filename (used for filtering) and seems true for clang SourceLocation too (it does a heuristic when creating CGDebugInfo which is more complicated than the one MLIR lowering does which just makes filename part of DIFileAttr the file part of path).

I can understand why you want it here as that is matching the output you are producing. And I could see folks arguing “well often directories are reused”, so locking 2x, doing 2 hash lookups, and always needing to mangle the strings whenever path is needed (which is the common case) is worthwhile in that it uses less memory while being less efficient. But I could see that same argument saying we need arbitrary splitting to allow for DAG of locations with at most N lookups rather than 1 or 2 (and additional arithmetic support would help - e.g., dwarf is the way it is for a reason). It’s very nice to have the common case be simple & cheap.

Why? For me, DIFileAttr is an attribute referring to a file location (its a full path being modeled), FileLineColLoc is an attribute referring to a file location (full path). One can argue that DIFileAttr in addition also captures specific way of interning into a debug table a full path name, but that’s for me in addition to the file path.

LLVM_DIGlobalVariable replicates 2 of the fields of FileLineColLoc, LLVM_DILexicalBlockFile I see replicated all fields of FileLineColLoc in 3 attributes, LLVM_DISubprogramAttr 2 of them.

Yes or better, populating a struct which abstracts over either form, so that we don’t need rfind in the most common case (even always contain an array of StringRef’s so one could arbitrarily split).

I see it the other way, “the DILocationAttr would hold the line/col information for the op” - tells me it would contain the line and col yes. I’d see it as being specific to an op wherever you create a DILocationAttr and so having these, and wherever its not specific to the op, it is at file level. So one would be creating new DIFileAttr’s for each given DILocationAttr. Which I admit isn’t ideal unless factored to avoid redundancy (LLVM_DILexicalBlockAttr works great with this, LLVM_DILexicalBlockFile not as well and LLVM_DISubprogramAttr poorly). But does identify a gap in that if one wants the same LLVM_DISubprogramAttr on multiple ops that are different locations, then it wouldn’t work to simply use that attribute in a loc and its nested file loc.

N lookup, its a linear search. But for me its more consistency question: if all filename’s are encoded in the same way, which then is serialized in a stable manner, which handles error emission correctly, which handles the common case usage most efficiently, why make it more expensive for the common case for the benefit of exception? The only argument I see here is that it might reduce memory usage by encoding common prefix as its own intern’d string, but none of the frontends do. I don’t actually see how one doesn’t start with the full path string already interned today except in the case of LLVM roundtripping (e.g., DIFile results in duplication in context for all frontends except roundtripping today). So we aren’t avoiding duplication, in fact interface here would have encouraged using form which creates duplication.

Agreed. I don’t even think the filename ever should be getting changed, so if correct at construction time, I don’t know how folks end up invalidating it (it should just be getting propagated surely, at worst put into all kinds of FusedLocs when doing fusions and the like).

The original proposal I notice also matches CudaTile_DILocAttr, I think it is a good way TBH. At worst one adds a verifier extra there to ensure consistency (saw you mentioned this). Especially if one wants to add the same LLVM_DISubprogramAttr onto multiple ops which are at different locations, then one can’t use the DIFileAttr inside of it for the ops, as its about the original language’s function (and that answers why one can’t do that). One should still use (FileLineColLoc, int) pair for DIFile IMHO as then filename is shared/not duplicated coming from all existing frontends even if not helping with error emission it reduces context size. You already mentioned you tried verifier and why it doesn’t work downstream (although I added attribute symbol use verification support “recently” which could work as then one can look up the symbols), but sounded like for LLVM dialect it would be direct as no value in the indirection & today already satisfies form that can be verified simply.

So long way it took me to get to: I like the original proposal more and a verifier would address the concern raised. I’d prefer that over interface. It just provides structured form over convention today, is local to LLVM, can actually reduce memory usage here (with DIFile changes), and doesn’t add overhead.

I’m always happy to improve things as appropriate :slight_smile:

I think the issue is the line/col bit - it feels strange to have line/col inside of DIFile even if they are zeros. For a filename, yeah sure I can totally agree that makes sense.

I’m not sure I understand this, I would expect ~1 DIFileAttr per ModuleOp?

TIL, thanks! I thought it was a sorted list but maybe that’s something else.

Yeah - we could store directory + filename + split point as StringRefParameter and a size_t in order to further optimize DIFile.

FWIW I was just poking around the definition of FileLineColRange and it’s just a StringAttr + some ints: llvm-project/mlir/include/mlir/IR/BuiltinLocationAttributes.td at main · llvm/llvm-project · GitHub (FileLineColLoc inherits from FileLineColRange)

Sounds good. We’ll stick with the original proposal and add a verifier to check that the FileLineColLoc in DILocationAttr matches the rendered filename from the attached scope’s DIFile (when you have one).