Page MenuHomePhabricator

[mlir][ods] Op::verify should not call OpAdaptor::verify
ClosedPublic

Authored by Mogball on Nov 2 2021, 11:36 AM.

Details

Summary

OpAdaptor::verify performs string lookups on an attribute dictionary. By
calling OpAdaptor::verify, Op::verify is not able to use cached attribute
identifiers for faster lookups.

Diff Detail

Event Timeline

Mogball created this revision.Nov 2 2021, 11:36 AM
Mogball requested review of this revision.Nov 2 2021, 11:36 AM

Update on binary size: this change actually reduces binary size slightly. The majority of *OpAdaptor::verify functions are stripped because they are never called.

Trying to create a common verify method which both *Op and *OpAdaptor call into reduces binary size but not as much as just leaving both functions separate. No visible performance impact, but it does make OpDefinitionsGen.cpp messier.

rriddle accepted this revision.Nov 2 2021, 9:10 PM

Do you have any compile time metrics for this? Seems like a good cleanup in general though.

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
461
2130–2133

Prefer early exit.

This revision is now accepted and ready to land.Nov 2 2021, 9:10 PM
jpienaar accepted this revision.Nov 3 2021, 8:39 AM

Thanks for checking on size, looks good thanks

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

I'm not sure I follow why opRequired check is needed here - this could show a bug elsewhere

Mogball marked an inline comment as done.Nov 3 2021, 9:49 AM
Mogball added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2138

When emitting a verifier inside an op, attribute lookups are (*this)->getAttr({0}AttrName()) and {0} must be the "getter" name of the attribute, e.g. operand_segment_sizes -> getOperandSegmentSizes. The generated file won't compile without this check.

jpienaar added inline comments.Nov 3 2021, 9:58 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
2138

For genNativeTraitAttrVerifier, I see attrGet being opGetAttr and adaptorGetAttr, and it seems that whether or not to use adaptorGetAttr or opGetAttr seems to be the same as opRequired here - and there is clear linkage between opGetAttr and adaptorGetAttr and op.getGetterName usage or not. So is that true? That if opRequired is specified then we have to use opGetAttr and vice versa. And then do we need to have both be specified at call sites?

Mogball updated this revision to Diff 384509.Nov 3 2021, 10:24 AM
Mogball marked 3 inline comments as done.

Review comments.

As for compile time, the effects of this change alone are pretty muted (<1%),
but coupled with subrange lookup (upcoming patch) the effects become more
noticeable (~2%).

Mogball added inline comments.Nov 3 2021, 10:26 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
461

Oh it's actually a std::string, even though it's compared against StringRef::npos ...

2138

That is true, but I've got a follow-up patch that cleans up all this op vs adaptor handling anyways.

This revision was automatically updated to reflect the committed changes.