Page MenuHomePhabricator

Enable useDefault{Type/Attribute}PrinterParser by default in ODS Dialect definition
ClosedPublic

Authored by mehdi_amini on Jan 17 2022, 4:42 PM.

Details

Summary

The majority of dialects reimplement the same boilerplate over and over,
switching the default makes it for better discoverability and make it simpler
to implement new dialects.

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 17 2022, 4:42 PM
mehdi_amini requested review of this revision.Jan 17 2022, 4:42 PM
rriddle added inline comments.Jan 17 2022, 4:45 PM
mlir/lib/IR/BuiltinAttributes.cpp
14

Are these necessary?

mlir/lib/IR/Location.cpp
11 ↗(On Diff #400672)

What is this used for?

mehdi_amini marked an inline comment as done.

Remove unnessary includes

mehdi_amini added inline comments.Jan 17 2022, 4:57 PM
mlir/lib/IR/Location.cpp
11 ↗(On Diff #400672)

It was needed originally before I disabled it in the .td file for this dialect.

The generated printer/parser uses some AsmParser defined there.

switching the default makes it for better discoverability

So too would making it unset by default (?) and flagging a warning at build time asking it to be explicitly set either way and explaining benefit of having it on (so no warning if 0). That is not a behavior change but pushes people in a better direction. Or it could simply be used to give folks a heads-up before flipping. Like a PSA but in code that they can see in their presubmit.

The dialects changed here can be done in separate changes from the default flips.

committing the dialect changes without changing the default would require to change the setting on each dialect individually back and forth, so I'd rather avoid the unnecessary churn here.

Tweak the ODS generators logic to have the decl/def generators in sync in their logic to decide when to emit or not the print/parse Type/Attribute methods.

rriddle accepted this revision.Jan 17 2022, 10:12 PM
rriddle added inline comments.
mlir/lib/IR/BuiltinAttributes.cpp
14

Unresolved?

mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
642 ↗(On Diff #400711)
This revision is now accepted and ready to land.Jan 17 2022, 10:12 PM
mehdi_amini marked 3 inline comments as done.

address comments

mlir/lib/IR/BuiltinAttributes.cpp
14

Strange, I thought I removed these...

This revision was landed with ongoing or failed builds.Jan 17 2022, 10:38 PM
This revision was automatically updated to reflect the committed changes.