Would it be possible to split the name into two parts? I.e. use the attribute name as a prefix in front of the sequential number during printing and then reliably parse the attribute name from it later?
The example from the first post could be printed as:
%foo_42 = xyz.add %40, %arg0 : i32
%foo_43 = xyz.mul %foo_42, %50 : i32
Does this make sense?
The LLVM implementation is virtually free for production: all the names are only kept in a debug build of the compiler. There is a flag on the LLVMContext to disable name preservation which ignores them (Value::setName() is a no-op then).
I always wanted us to have names like LLVM, but my main motivation was not to be able to write better tests (this is nice of course) but rather because it helps debugging. When you use something like “print-after-all” and track the IR after every transformations it is nice to have something to anchor to.
(Actually I even recently implemented a pass in MLIR that add an attribute with a unique id on every operation to be able to track them before/after a specific pass I was debugging, I don’t understand how we can debug without that…)
I understand that this is hard to get at in MLIR and the tradeoff in complexity mean it won’t happen, but still sad…
Parenthesis: LLVM SSA naming does not have effect on the core semantics of the IR either.
This is mostly orthogonal to your proposal though, so I’m not arguing against what you’re proposing here.
And to come back to the original proposal, if we’re going in this direction, then why don’t we just leave the entire op parsing to the operation custom parser?
The operation parser would be responsible to define new SSA value names for its results by calling back into the OpAsmParser, with the only restriction that the number of values created should match the return type of the newly created op.
It’d be fun to write even more DSL-like code in MLIR this way 
I’m pretty sure that we can achieve this all today without changing anything in the core (i.e. don’t add a setName and a new name field). Just have the asmprinter look for a “std.name” string attribute, and use it to override the default name if present. This is effectively what I’m talking about in this thread, but would apply to all operations everywhere.
I posted a patch in phabricator that adds the support I need to the parser/printer interfaces. It is super simple, just one method each. Assuming this is ok, I’ll take a look at improving the FunctionLike parsing/printing logic in a follow-up patch.
I don’t really see how this provides the features you get with LLVM?
First attributes aren’t maintained through transformations right now, we don’t have a way to propagate them. You could argue that this new attribute would be “magic” and “blessed” to be specifically handled by every transformation.
However this still does not provide the property that LLVM has that the name is overly stable after creation (it gets suffixed automatically when needed) and this is what allow to track a value over its lifetime through the pass pipeline.
Also, I don’t think you went all the way to what I mention above with leaving the entire operation custom parser in control: you still have
This does not invalidate the use you have for this, but I believe it just does not solve the other use cases.
Right now the only thing that makes me reluctant to your proposal is that by making the SSA name semantically important (or “load bearing” as you said), it isn’t clear to me that we can still freely work on features that would make use of the names for other non-semantically important purpose (like debugging).
I don’t understand. SSA names in LLVM are definitely not stable. How are they any different than optional metadata in LLVM or dialect-specific names in MLIR? There may be some small difference, but they seem extremely close to me. They aren’t generally maintained by transformations.
You mention that SSA names in LLVM have very little cost. This is true in that there is just one bit in llvm::Value and the names themselves are stored in an on-the-side table. The cost of the LLVM design is threefold:
setName() which then ignores the name.In any case, I’m not claiming that LLVM style names have zero value. I’m claiming that they can be approximated fairly closely and that the complexity of adding something like they would have to be justified by a significant benefit.
+1 that’s super useful.
An alternative, more heavyweight way to get that is to go all-in on pass pipeline debugging: structured print-before-all/print-after-all output, tracking stable names in locations, and a web UI that knows how to deal with that and query. E.g. you go to IR before one pass, click on an operation you would like to track, and then it highlights that and all operations derived from it as you scroll through all the passes.
@benvanik showing me something like that for a previous thing he made and it made me drool. I seem to recall XLA having something similar as well?
It sounds to me that for tracking IR over the course of optimizations, locations are a natural place to store the identifying information – we already have to track them, and it’s easy to squirrel more information inside locations if needed.
How would we know which op’s custom parser to invoke? Currently, we rely on being able to parse a closed universe of result syntaxes to get to the op name to dispatch to the custom parser if needed.
We would have to support results after the keyword, ala:
my.op %42 = ...
This would allow the ‘my.op’ parser to format its results in completely arbitrary ways. It could even eliminate the %'s for example if there were some reason. I’m not saying this is a good idea, just saying it would be possible.
This could be added into the current world if there were a need: we could allow the asmparser to add results that don’t have already assigned names.
They aren’t entirely stable in the sense that inlining would trigger a rename for example, or unrolling as well, but you’re in general able to track their prefix because of the uniquing on insertion.
Anyway I’m not arguing for going all-in on these features right now, my main concern here is the “load bearing” part (see below).
Right: so if we agree that any “approximation” to this debug feature is desirable, I wonder how nice does it play with the “load bearing” aspect. You floated the idea of just having the asmprinter look for a “std.name” string attribute, and use it to override the default name if present :
This “debug oriented” view of the feature however does not seems entirely compatible with what you want (names are required and preserved for correctness).
Mehdi, I’m not proposing a standardized approach here or any specific semantics in the core. I’m proposing a couple of tiny extensions (two new methods) that allow dialects to do things local to themselves.
You can see the details here. I think the “load bearing” terminology has been misunderstood, which was my mistake. Sorry about that, I’ll refrain from that terminology in the future.
-Chris
Apparently I am not clear either, because I believe I looked at your patches and I thought I understood your proposal: my impression is that if we go with what you propose we are closing the future possibility to use the SSA name for debugging purpose.
This seems to me to be inherent to giving the control on these names to the operation, in an observable way.
Aha, yeah I didn’t understand that point at all, thank you for clarifying! To restate, you’re saying that we could have MLIR reserve the result naming specifiers for a global purpose, one that crosscuts all dialects and which cannot be customized by the dialect author.
I’d personally be opposed to this direction: not just because I have an application that would break :-), but also because I think it cuts against the larger design point we have: The “pretty” MLIR syntax can be made nice to work with in domain specific ways - even allowing operand lists and other things to be encoded in completely different ways.
I see this as exactly equivalent to the idea of MLIR dialects: on the one hand we want compiler engineers to have infinite design space to be able to implement clever or crazy things. On the other hand we want to encourage standardization – no, it is not a good idea to innovate on operand list syntax even if you can!
This balance is good in my opinion because we cannot forsee all future possibilities and applications, and the only constant in life is change. By providing a flexible and extensible design, we can increase the odds that the core infra is future proof and scales to new domains.
-Chris
Today the result naming specifiers is de-facto reserved by the fact that we don’t have the API you want to provide, so at least “MLIR reserve the result naming specifiers” looks like the current status-quo to me!
Yes. How do you see a debug feature like the one you mentioned before otherwise?
You wrote: Just have the asmprinter look for a “std.name” string attribute, and use it to override the default name if present. This is effectively what I’m talking about in this thread, but would apply to all operations everywhere. ; which I took as a standardized feature of std.name for debug purpose.
First, let me explicitly recognize the value I see in LLVM-style SSA names. 1) They provide a way to put a somewhat stable identifier on an operation, so you can see the operation move through the passmanager and see it in debug dumps. 2) It makes it much easier to reason about the uses of the operation results downstream, because the names show up in the operand lists of users.
The semantics I’m suggesting would be pretty simple: the std.name attribute would always be printed in the verbose form. If there is no “pretty” name specified for a result, and if the operation producing it has an std.name attribute, use that attribute for the pretty name.
This means that a) You’re not impinging on operation semantics, only providing a default when none other is provided, b) the attribute is always printed in the normal way.
This is close (but again, not exactly the same) as the LLVM semantics, where it provides a best effort name propagation system. It would not work in weird dialects that want to do things with the names, but those are exactly the dialects where you don’t need this functionality! In that case, you’d get benefit #1 but not benefit #2.
-Chris
I’ve updated the patch and it is ready to land, any further objections?
Once that is in, I will do a few NFC cleanups to the codebase that I’m noticing.
I just want to finish the discussion before you open-up a door that we can’t close back.
OK that seems like a tradeoff I can live with!
However it seems to me that we would need to able to identify if an operation is sensitive to the SSA name or not, otherwise using a name where there was none can change the round-trip aspects. This can likely be handled by having your feature behind an op interface?
Yes, definitely, that’s why I pinged the thread.
I think you’ve got the parity of this inverted. A debugging feature should not interfere with the spelling of operations that care about their names, it isn’t the parsing logic in the operation that should know about the debugging feature.
I can imagine two different ways to handle this debugging feature in the future:
Simplest and most general is to add a prefix to the debugging names, e.g. all debugging names end up looking like %$foo. Ops that want to use the names would need to know that the $ is an invalid prefix for them to use, which seems very reasonable.
Alternatively, when the debugging feature is implemented, we can add an operation trait bit (no need for an op interface) saying “this operation/dialect” cares about names. The debug name feature thingy would then avoid touching the name for those operations.
Does this make sense to you? Both of these are possible to add in the future, this patch does not affect either direction.
-Chris
Absolutely! Actually when I wrote “we would need to able to identify if an operation is sensitive to the SSA name or not” I meant that the debugging feature would have to know if an operation cares or not about its result’s names, and not touch them in this case. This is basically your option 2 if I understand you correctly.
Your option 1 made me think about a slightly tweaked one on the prefix. The reserved first character could be used it as a delimiter to mark the end of the debug name when present, so that the “load bearing” name would still be preserver and exposed:
%42 = xyz.add %40, %arg0 {xyz.name = "foo", std.debugName = "blah"} : i32
%43 = xyz.mul %42, %50 {xyz.name = "bar"} : i32
would get printed:
%$blah$foo = xyz.add %40, %arg0 : i32
%bar = xyz.mul %$blah$foo, %50 : i32
Anyway, this is just future work. As long these options are kept possible I am fine with your current patch.
Thanks Mehdi, I landed the patch. I appreciate the feedback,
-Chris