-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[mlir] Enable setting alias on AsmState. #153776
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -560,6 +560,15 @@ class AsmState { | |
FallbackAsmResourceMap *map = nullptr); | ||
~AsmState(); | ||
|
||
/// Add an alias for the given attribute. The alias will be used as a | ||
/// suggestion when printing. The final alias may be modified to resolve | ||
/// conflicts. | ||
void addAlias(Attribute attr, StringRef alias); | ||
|
||
/// Add an alias for the given type. The alias will be used as a suggestion | ||
/// when printing. The final alias may be modified to resolve conflicts. | ||
void addAlias(Type type, StringRef alias); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also curious about the use-case: you can't know all the possible attributes that will be created by the compiler ahead of time, making this feature quite limited in practice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The application here in fact does. E.g., the type itself & dialect doesn't know the use, but the application does. So, as an upstream example, you have a tensor of size of quantized type with loads of params. The dialect and generic hooks can't do much as this is just regular tensor from builtin dialect. But in the application, it knows the user considers it the "jtensor" (to pick a name) that is used everywhere. The builtin dialect assigns no special meaning to it, its only in the application/user context where it has one. So enables giving concise & application relevant aliases even where no generic ones apply. And here it is often the mapping from entry function signature that captures this mapping, so it is easy to populate mapping. Its not something generic for the type that is, so a type attribute interface would not match. That being said, I did consider creating a aliasinghelper dialect with interface and injecting, but this would then require mutating map before printing (in a thread safe manner) and I don't think one can override that of another dialect like this (could be wrong, this may only be true for those with final alias set). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see where this is coming from. One annoying thing I can already see is that none of the tooling would round-trip this, there is no way to configure a tool properly for this, and even when you have an application that wants to set this kind of things. The fact that it is an API on the AsmState makes it so that it wouldn't compose with a lot of things (if you interact with other components, the AsmState is not something in general part of the API boundaries) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. Now I can make it so :) (which would remove it from API here). And that is by way of adding something like a alias attribute to the top level op - which is just a dictionary attribute of name & attribute. So the printer can query if attribute on op being printed and then populate as here. This would most likely be a discardable attribute though and (downside) would fix a naming convention. E.g., if op has certain named attribute ("attribute_type_alias_map" ? something not likely to be in use already and self-documenting) of dictionary attr kind, then use it. Probably nicer, more composable but at cost of convention. |
||
/// Get the printer flags. | ||
const OpPrintingFlags &getPrinterFlags() const; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it play with the existing interfaces that can already alter this? What is the order of priority?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if we want a contract on ordering vs it being a hint, so left it unspecified. Currently this will override - but it is in order of which alias was first specified (e.g, two addAlias calls for same Attribute will result in the first being used). Which corresponds to the user/application being able to dictate alias first and then goes down the regular alias path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing is what is the behavior of repeated calls to the API with the same attribute and different alias names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mention that above, first one is set, successive calls ignored. Or do you mean I should add to docstring?