LLVM Discussion Forums

[RFC] Better support for dialects that want to make SSA names "load bearing"

On a minor note, I’m trying to understand why the roundtripping breaks when the autorenaming is done correctly. The uses don’t get renamed correctly?

Why even have xyz.name when you have locations? Just have a NamedLoc. If you then need more info, you could always just create a FusedLoc with a NamedLoc and other loc (and metadata to make it clear it is the load bearing name if needed). You’ll notice we have used locations pretty extensively in TensorFlow and TFLite dialect where name attribute was pretty pervasive. It avoids needing to teach CSE, for example, about name attribute.

Oh right, I forgot that part. I added a section above name “Problem: Custom op parser needs to be able to query names of op results”. This can be handled by adding one method to OpAsmParser.

All I meant by “load bearing” (which I admit is not a great term :slight_smile: ) in that the parser and printing hooks for operations have access to the parsed/printed result names of the current operation, allowing them to implement custom behavior. That’s all this RFC boils down to.

Uses get renamed correctly. Recall that these names are semantically meaningful. Therefore if this operation:

%42 = xyz.add %40, %arg0 {xyz.name = "foo"} : i32

Got printed out as:

%foo_42 = xyz.add %40, %arg0 : i32

Then when it is parsed back in, the xyz.name attribute would be set to “foo_42” instead of “foo”. This is what I meant by “breaking round tripping”. The solution for this is to explicitly place the attribute in its attribute dictionary in this narrow case. This eliminates the syntactic niceness, but maintains correctness.

I hadn’t thought about using locations for this, but I don’t think this helps. These are semantically part of the IR, so they should block CSE and other things. Also, locations aren’t printed by default by mlir-opt, which is another bad thing. Furthermore, when locations are enabled, we would end up with the same verbosity problem.

In my view, the pretty syntax features that MLIR provides are a way for op/dialect implementations to eliminate redundancy. This is fundamentally why std.add wants to print a type of “i32” instead of “(i32, i32) -> i32”. Eliminating redundancy increases clarity, increases understandability, and makes it easier to write testcases. My goal here is to do the same thing with a string attribute, encoding it into syntax that already exists.

Makes sense now - thanks!

This is where the names now have semantic meaning. This makes the result assignment part of the custom assembly format, which it currently is not. For example, how would such an operation handle the many different ways of naming a result? Consider the example of a simple variadic operation, there could be zero names, one name, or even multiple names assigned to the result of that operation. What does the parser of the operation do in any of these cases? handle all of them? fail?

A couple potential interactions of the proposal. Would like to know what folks think.

For a while I’ve been wanting the IR to better round-trip names, which makes testing a lot easier.

E.g. currently this function:

func @f(%arg0: f32) {
br ^loop(%arg0: f32)
^loop(%loop: f32):
br ^loop(%loop: f32)
}

Round-trips as:
func @f(%arg0: f32) {
br ^bb1(%arg0 : f32)
^bb1(%0: f32): // 2 preds: ^bb0, ^bb1
br ^bb1(%0 : f32)
}

That makes testing kind of annoying. It would be nice if the names of blocks and block arguments were preserved.

Currently I see great productivity gains by just being able to hardcode arg0 in tests (reduces the amount of filecheck capturing needed), and preserving a custom name there would make tests even more readable. Block names and SSA value names in the body being preserved (as long as we know that the pass under test isn’t touching the associated ops) could really help a lot with making tests easier to write and more readable.

I’m curious if we want to push further in this direction. If we do, then it feels like it would interact with what Chris wants here. I really think this could significantly reduce test maintenance costs in the MLIR ecosystem.

Also, Chris, are the names logically on the ops, or on the Value’s? (or is there an implicit single-result assumption making the question moot?)

What are the semantics of two ops/values with the same name? I’m failing to come up with a load-bearing semantic that would make sense in that case.

On the specific IR required here, I’m also worried that it won’t be obvious at a glance at the IR which names are load bearing vs not. Could we use a different sigil or something to indicate it?

Btw, for some prior art in this domain, IREE’s vm.import op does have load-bearing names on the arguments. (done with a vm.name arg attribute)

In my opinion, the parser should hand it to the operation’s parser implementation (when queried through the new method) and the operation parser can decide what to do with it.

I think it would be a bad idea to do this. LLVM does this, and many people find it to be appealing, but this is a bad tradeoff for production. Those strings are expensive to propagate and need to be uniqued, which makes IR manipulation (the core part of the compiler) slow.

The proposal I’m floating here are just a few tiny extensions that make existing structures more flexible. They don’t have any effect at all on the core semantics of the IR.

As River points out, an asmparser hook that wants to do something with names should be prepared to handle the various spellings. If it is %foo:4 = or %foo,%bar =, then the parser hook should know that.

This should remain a parse error. The operation parser hook should not be able to override core aspects of the IR like dominance rules etc.

How is this different than any other dialect specific asmparser/printer rules? The things we do for region based control flow statements are way more “interpretive” than this :slight_smile:

-Chris

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 :slight_smile:

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:

  1. a complexity cost because the behavior of the compiler changes significantly based on a big switch (whether names are enabled or not). This isn’t (supposed to be) a semantic change, but it affects testing and other things.
  2. the other thing is that that bit does get tested in a bunch of places, and that has a non-zero runtime cost. Likewise the things that add suffixes sometimes create the suffixed name to call setName() which then ignores the name.
  3. a fairly significant runtime cost when names are enabled, because it is expensive to manipulate the names in the side table.

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.