Page MenuHomePhabricator

[mlir] Add option to outline large elementsattrs.
Needs RevisionPublic

Authored by jpienaar on Jan 17 2022, 7:17 AM.

Details

Summary

Enable printing elementsattr out of line (use aliases) if large. Follows
the elide logic mostly (outline if larger except if splat) with the
exception that setting max elements to 0 always outlines.

Diff Detail

Event Timeline

jpienaar created this revision.Jan 17 2022, 7:17 AM
jpienaar requested review of this revision.Jan 17 2022, 7:17 AM
jpienaar updated this revision to Diff 400544.Jan 17 2022, 7:20 AM

Update variable name

LG

mlir/lib/IR/AsmPrinter.cpp
133

Compare to mllir-elide-elementsattrs-if-larger this one does not break round-trip, so we should have a default value, something like 20 maybe?

rriddle added inline comments.Jan 17 2022, 11:48 AM
mlir/lib/IR/AsmPrinter.cpp
677–685

nit: Drop else after return.

jpienaar updated this revision to Diff 400686.Jan 17 2022, 5:59 PM

Remove unnecessary else post return

jpienaar marked an inline comment as done.Jan 17 2022, 5:59 PM
jpienaar added inline comments.
mlir/lib/IR/AsmPrinter.cpp
133

That is true, but it can make tests more difficult: by making alias it removes the direct, inline association and so makes the FileCheck pattern more complicated to write. As one has to match first on alias (which is now via DAG) then use the matched alias in the instruction match. I find that more cumbersome in tests (I do find it much nicer in regular dumps as I very rarely read the constants). Now when we get our test-expr syntax, different story ;-)

I'd prefer to keep this optional to ease test authoring.

mehdi_amini added inline comments.Jan 17 2022, 6:09 PM
mlir/lib/IR/AsmPrinter.cpp
133

A test with a large attribute that needs to be matched is likely rare, and if it happens then the option can be disabled explicitly in the test.

Let's pick the right default for the common case.

jpienaar added inline comments.Jan 17 2022, 8:41 PM
mlir/lib/IR/AsmPrinter.cpp
133

The right default depends on the use case. In particular I like the current format for all tests. While I sometimes like aliases for debugging and there setting 0 or 1 when looking at structure is useful, while tracking data-dependent optimization something like 8 or 32 could work but I'd probably just turn off line wrapping if I care about any constant value (keeps info local, avoid redirection). Now, reproducers I think this _could_ (not verified) reduce the size, so there making it default 0 could be good. The current default (of not printing out of line) is the preferred default for me in the majority of use cases.

mehdi_amini added inline comments.Jan 17 2022, 9:15 PM
mlir/lib/IR/AsmPrinter.cpp
133

The right default depends on the use case. In particular I like the current format for all tests.

Sure, but we're talking about large constants...
Can you point me to a variety of tests in the code base that include checks on large constants?

The current default (of not printing out of line) is the preferred default for me in the majority of use cases.

I strongly disagree here. I don't understand the rational for seeing the "majority of use cases" preferring to duplicate a large constant over and over and over in the output.
It always makes it unreadable as far as I can tell.

mehdi_amini requested changes to this revision.Jan 17 2022, 9:16 PM

(until we sort out the right default)

This revision now requires changes to proceed.Jan 17 2022, 9:16 PM
rriddle added inline comments.Apr 3 2022, 9:02 PM
mlir/lib/IR/AsmPrinter.cpp
133

Should we just align this with the default for printing in hex?

Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2022, 9:02 PM
mehdi_amini added inline comments.Apr 3 2022, 10:27 PM
mlir/lib/IR/AsmPrinter.cpp
133

We could add a default for the HEX printing as well, but that seems to me like a discussion we can or even should consider separately: adding a default here is intended to improve IR readability, while adding a default to the hex option does not operate on the same axis as far as I can tell.

mehdi_amini added inline comments.Apr 3 2022, 10:37 PM
mlir/lib/IR/AsmPrinter.cpp
133

Ah! I stopped reading at getNumOccurrences() line 262...
Yeah then that seems an obvious candidate indeed: there is no point in keeping inline attributes we already judge too long to be readable and we print in hex.