Page MenuHomePhabricator

[mlir][ODS] Fix copy ctor for generate Pass classes
ClosedPublic

Authored by vinograd47 on Jun 15 2021, 8:13 AM.

Details

Summary

Redirect the copy ctor to the actual class instead of
overwriting it with TypeID based ctor.

This allows the final Pass classes to have extra fields and logic for their copy.

Diff Detail

Event Timeline

vinograd47 created this revision.Jun 15 2021, 8:13 AM
vinograd47 requested review of this revision.Jun 15 2021, 8:13 AM

Whoa, great catch. Only request: please merge the unit test into the existing test/lib/Pass stuff. Thank you!

Only request: please merge the unit test into the existing test/lib/Pass stuff.

I'm not sure. As far as I see, test/ folder contains LIT-based tests and test/lib contains auxiliary libraries for those tests. unittests/ folder contains GTest-based tests for some components, like mlir-tblgen. And the new tests relates to mlir-tblgen pass generation functionality. That's why I've put it into unittests/TableGen folder.

Do we need a unit test at all here? If I understand the patch correctly, you're saying that the canonicalize options aren't being copied over correctly when a pass is cloned. If that is true, then we can exercise this with a standard filecheck test of the canonicalize pass.

Unit tests impose an outsized burden on the project because they increase link and build time when doing check-mlir. Filecheck tests of existing tools are much cheaper.

vinograd47 edited the summary of this revision. (Show Details)

Removed confusing comment about Canonicalization pass.

@lattner Sorry, my comment about Canonicalization pass was wrong and confusing (removed it).

We faced with this issue in our internal project. We have pass, which has extra fields not configurable from command line options. Those fields are used in production code, but are not needed in testing/debugging tools like mlir-opt. Due to the issue with copy ctors, those fields were lost during pass cloning.

Initially I thought that the Canonicalization pass will have the same issue. But after more careful look I've found that the the config options for greedy pattern driver are available via command line options and the pass constructor initializes GreedyRewriteConfig object from them.

Nevertheless, I still think that the missing copy ctor call is an issue. The mlir-opt tools and command line options are not mandatory and external projects might not use them.

lattner accepted this revision.Jun 19 2021, 8:29 AM

Ok, LGTM, thanks!

This revision is now accepted and ready to land.Jun 19 2021, 8:29 AM
This revision was automatically updated to reflect the committed changes.