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.
Details
- Reviewers
rriddle mehdi_amini
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
mlir/lib/IR/AsmPrinter.cpp | ||
---|---|---|
677–685 | nit: Drop else after return. |
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. |
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. |
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. |
mlir/lib/IR/AsmPrinter.cpp | ||
---|---|---|
133 |
Sure, but we're talking about large constants...
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. |
mlir/lib/IR/AsmPrinter.cpp | ||
---|---|---|
133 | Should we just align this with the default for printing in hex? |
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. |
mlir/lib/IR/AsmPrinter.cpp | ||
---|---|---|
133 | We already have a default BTW: |
mlir/lib/IR/AsmPrinter.cpp | ||
---|---|---|
133 | Ah! I stopped reading at getNumOccurrences() line 262... |
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?