This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ods] Enable emitting getter/setter prefix
ClosedPublic

Authored by jpienaar on Oct 3 2021, 4:22 PM.

Details

Summary

Allow emitting get & set prefix for accessors generated for ops. If enabled, then the argument/return/region name gets converted from snake_case to UpperCamel and prefix added. The attribute also allows generating both the current "raw" method along with the prefix'd one to make it easier to stage changes.

The option is added on the dialect and currently defaults to existing raw behavior. The expectation is that the staging where both are generated would be short lived and so optimized to keeping the changes local/less invasive (it just generates two functions for each accessor with the same body - most of these internally again call a helper function). But generation can be optimized if needed.

I'm unsure about OpAdaptor classes as there it is all get methods (it is a named view into raw data structures), so prefix doesn't add much.

This starts with emitting raw-only form (as current behavior) as default, then one can opt-in to raw & prefixed, then just prefixed. The default in OpBase will switch to prefixed-only to be consistent with MLIR style guide. And the option potentially removed later (considered enabling specifying prefix but current discussion more pro keeping it limited and stuck with that).

Also add more explicit checking for pruned functions to avoid emitting where no function was added (and so avoiding dereferencing nullptr) during op def/decl generation.

See https://bugs.llvm.org/show_bug.cgi?id=51916 for further discussion.

Diff Detail

Event Timeline

jpienaar created this revision.Oct 3 2021, 4:22 PM
jpienaar requested review of this revision.Oct 3 2021, 4:22 PM

Cool, I'm super excited to see this!! The ERROR_IF_PRUNED additions seem logically unrelated to this patch, but are valuable and aren't too distracting.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
530

I'd recommend renaming "read" to "isGetter" and making it a parameter instead of a template argument. No need to duplicate all this code, particularly in tblgen

556

Random wishlist: I wish tblgen had some exception lists for common names like "lhs" and "rhs" so that it would turn it into "getLHS" instead of "getLhs". Not really related to this patch though, since this happens with the existing AttrDef accessors.

571

I'd recommend naming this getGetterNames and getSetterNames

589

What is the LINE doing here? This will always expand to line 589 since that is where it is in this file, that can't be what you intend?

817–818

I'd recommend setterName and getterName since getName sounds like an accessor itself and not a noun.

rriddle added inline comments.Oct 5 2021, 11:22 AM
mlir/include/mlir/IR/OpBase.td
240–242

Can you document these?

mlir/include/mlir/TableGen/Dialect.h
89

Is the hex necessary here? Why not just 0, 1, 2?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
589

This is a macro, so I would expect LINE to expand properly (https://godbolt.org/z/erG8cG8d6)

lattner added inline comments.Oct 5 2021, 11:47 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
589

Yep my mistake got it.

jpienaar updated this revision to Diff 378688.Oct 11 2021, 8:46 AM
jpienaar marked 9 inline comments as done.

Renamed getter/setter get methods and document enums.

jpienaar updated this revision to Diff 378690.Oct 11 2021, 8:50 AM

Remove change to shapebase which will be made in follow up

rriddle accepted this revision.Oct 11 2021, 6:22 PM

Did a quick additional scan, but LG in general

mlir/include/mlir/IR/OpBase.td
299

with no prefix?

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
531

nit; Spell out auto here.

549
This revision is now accepted and ready to land.Oct 11 2021, 6:22 PM
jpienaar updated this revision to Diff 378863.Oct 11 2021, 8:17 PM
jpienaar marked 3 inline comments as done.

Update comments and spelled out type.

lattner accepted this revision.Oct 11 2021, 10:29 PM

I'm very excited about this work, great job!

mlir/include/mlir/IR/OpBase.td
243

nit: 80 column violation

jpienaar updated this revision to Diff 379528.Oct 13 2021, 2:55 PM
jpienaar marked an inline comment as done.

Fixing to line length & rebase

This revision was automatically updated to reflect the committed changes.