Page MenuHomePhabricator

Add a new interface method `getAsmBlockName()` on OpAsmOpInterface to control block names
ClosedPublic

Authored by mehdi_amini on Dec 15 2021, 10:46 PM.

Details

Summary

This allows operations to control the block ids used by the printer in nested regions.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 15 2021, 10:46 PM
mehdi_amini requested review of this revision.Dec 15 2021, 10:46 PM
rriddle added inline comments.Dec 15 2021, 10:51 PM
mlir/include/mlir/IR/OpAsmInterface.td
66–67

The phrasing of this is not the same as others, can you restructure this message to have a bit more detail? and e.g. show an example?

mlir/lib/IR/AsmPrinter.cpp
1056–1059

What's the runtime cost of using std::string for all blocks now?

1062

Please use proper sentences for comments.

Improve description

mehdi_amini marked 2 inline comments as done.Dec 15 2021, 11:07 PM
mehdi_amini added inline comments.
mlir/lib/IR/AsmPrinter.cpp
1056–1059

I'd say "not measurable compared to the rest of the infra involved in printing stuff"?

The real cost isn't in the block you highlighted actually, it is above: the loop with the while(1) will be invoked when printing the blocks in a function for example. That's an O(N^2) loop in terms of number of blocks...

1062

Except the leading upper case, what's missing?
(this is left from the LHS of the diff FYI)

mehdi_amini marked an inline comment as done.

Add the O(N2) behavior

rriddle added inline comments.Dec 15 2021, 11:09 PM
mlir/lib/IR/AsmPrinter.cpp
1056–1059

Have you measured it? Can we avoid all of this complexity in the common case (no override)?

1062

(Just meant the upper case)

mehdi_amini added inline comments.Dec 15 2021, 11:11 PM
mlir/lib/IR/AsmPrinter.cpp
1056–1059

(fixed the O(N2) right now)

Have you measured it?

No, but .... really?

Can we avoid all of this complexity in the common case (no override)?

Sure, I'll change the interface to return a callback...

mehdi_amini added inline comments.Dec 15 2021, 11:13 PM
mlir/lib/IR/AsmPrinter.cpp
1056–1059

(Actually that won't change the fact that we'll be using strings instead of unsigned in the map)

Use a factory to detect cases where the interface isn't overloaded

Mogball requested changes to this revision.Dec 16 2021, 8:36 AM
Mogball added inline comments.
mlir/include/mlir/IR/OpAsmInterface.td
92

Why is using std::string necessary?

mlir/lib/IR/AsmPrinter.cpp
850

Values have a separate map for numeric IDs and string names. Why not do the same for blocks? Then it might be possible to avoid some of the penalty for ops that don't implement the interface method.

1049–1050

This looks like an inlined copy of SSANameState::uniqueValueName

1051
This revision now requires changes to proceed.Dec 16 2021, 8:36 AM
mehdi_amini marked an inline comment as done.Dec 16 2021, 4:28 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/OpAsmInterface.td
92

It is an ownership problem: if you build a string dynamically on the other side of the interface, who owns it?

mlir/lib/IR/AsmPrinter.cpp
850

It just seems overly complex for something that seems extremely unlikely to me to every be a bottleneck anywhere.

Note also the tradeoff: here we precompute the string for the ID, while before you recompute a string for the ID on the fly. It's not clear to me that recomputing on the fly the integer serialization is necessarily better. But in any case these are not the bulk of the processing where we're talking about streaming a lot of string to an abstract stream implementation (regardless of the stream destination: string, file, console, socket, ...).

1049–1050

Yes: uniqueValueName has the "SSA value scope" I believe while this is for block names (which don't conflict with SSA value name I believe?).
Are you thinking that this logic should be refactored and shared somehow?

mehdi_amini marked an inline comment as done.Dec 16 2021, 4:28 PM
mehdi_amini added inline comments.
mlir/include/mlir/IR/OpAsmInterface.td
92

(the alternative would be a "StrAttr" instead I guess?)

Mogball added inline comments.Dec 16 2021, 5:12 PM
mlir/include/mlir/IR/OpAsmInterface.td
92

Why not use the usedNameAllocator?

mlir/lib/IR/AsmPrinter.cpp
1049–1050

Are you thinking that this logic should be refactored and shared somehow?

If possible, yeah.

mehdi_amini added inline comments.Dec 16 2021, 5:41 PM
mlir/include/mlir/IR/OpAsmInterface.td
92

You mean pass the allocator as an argument? And then return a StringRef?

Sure we could do this as well, that seems just overly convolute to me for "block names".

Mogball added inline comments.Dec 16 2021, 6:30 PM
mlir/include/mlir/IR/OpAsmInterface.td
92

I'm okay with std::string, but given that value names have been done a particular way, I find it strange that block names are done in a different way.

mehdi_amini added inline comments.Dec 16 2021, 6:32 PM
mlir/include/mlir/IR/OpAsmInterface.td
92

We can add more indirection and pass a callback to the callback so that the code would call setNameFn like the getAsmResultNames above?

92

(some non-negligible difference I make here is that the number block IDs in a typical IR is a small fraction of the number of SSA values)

Rework the flow entirely, align the callback with getAsmResultNames()

Mogball accepted this revision.EditedDec 20 2021, 7:20 AM

Thanks! Otherwise LGTM

mlir/lib/IR/AsmPrinter.cpp
2688

Should this also sort by block name as well as ordering? Named blocks with ordering := -1 might be printed in an unstable order.

This revision is now accepted and ready to land.Dec 20 2021, 7:20 AM
mehdi_amini added inline comments.Dec 20 2021, 11:13 AM
mlir/lib/IR/AsmPrinter.cpp
1041

Here we always define an ordering Jeff: even for block that have a name.

Mogball added inline comments.Dec 20 2021, 11:19 AM
mlir/lib/IR/AsmPrinter.cpp
1041

Ah, I missed this. Thanks.

Rebase.

I don't need this right now, but it is ready to land if we think it is a desirable addition!

rriddle accepted this revision.Feb 8 2022, 12:25 AM

I think I would be okay landing this given that it is not super intrusive, and opens up interesting possibilities for users. That being said, I don't have a concrete use in mind right now.

mlir/lib/IR/AsmPrinter.cpp
980

Do the other "INVALID" things have an underscore in the name? I can't remember.

1035–1039

We could just say if the explicit name is empty, we use the ordering for a ^bbN name instead.

This revision was landed with ongoing or failed builds.Feb 11 2022, 12:46 AM
This revision was automatically updated to reflect the committed changes.