Page MenuHomePhabricator

[mlir][AsmPrinter] Refactor printing to only print aliases for attributes/types that will exist in the output.

Authored by rriddle on Oct 30 2020, 4:13 PM.



This revision refactors the way that attributes/types are considered when generating aliases. Instead of considering all of the attributes/types of every operation, we perform a "fake" print step that prints the operations using a dummy printer to collect the attributes and types that would actually be printed during the real process. This removes a lot of attributes/types from consideration that generally won't end up in the final output, e.g. affine map attributes in an affine.apply/affine.for.

This resolves a long standing TODO w.r.t aliases, and helps to have a much cleaner textual output format. As a datapoint to the latter, as part of this change several tests were identified as testing for the presence of attributes aliases that weren't actually referenced by the custom form of any operation.

To ensure that this wouldn't cause a large degradation in compile time due to the second full print, I benchmarked this change on a very large module with a lot of operations(The file is ~673M/~4.7 million lines long). This file before this change take ~6.9 seconds to print in the custom form, and ~7 seconds after this change. In the custom assembly case, this added an average of a little over ~100 miliseconds to the compile time. This increase was due to the way that argument attributes on functions are structured and how they get printed; i.e. with a better representation the negative impact here can be greatly decreased. When printing in the generic form, this revision had no observable impact on the compile time. This benchmarking leads me to believe that the impact of this change on compile time w.r.t printing is closely related to print methods that perform a lot of additional/complex processing outside of the OpAsmPrinter.

Diff Detail

Event Timeline

rriddle created this revision.Oct 30 2020, 4:13 PM
rriddle requested review of this revision.Oct 30 2020, 4:13 PM
mehdi_amini added inline comments.Oct 30 2020, 7:29 PM

Why is this useful?

mehdi_amini accepted this revision.Oct 30 2020, 8:04 PM

LGTM overall

This revision is now accepted and ready to land.Oct 30 2020, 8:04 PM
rriddle updated this revision to Diff 302405.Nov 2 2020, 2:10 PM
rriddle marked an inline comment as done.

Resolve comments


Callers of this method rely on the output somewhat resembling an SSA value name, so to maintain this invariant we need to emit something even if it inevitably goes unused. Added a comment.

mehdi_amini added inline comments.Nov 3 2020, 3:28 AM

OK! Still LGTM :)

This revision was landed with ongoing or failed builds.Nov 9 2020, 10:03 PM
This revision was automatically updated to reflect the committed changes.