This is an archive of the discontinued LLVM Phabricator instance.

[mlir-tblgen] Avoid ODS verifier duplication
ClosedPublic

Authored by Chia-hungDuan on Jun 16 2021, 4:47 AM.

Details

Summary

Different constraints may share the same predicate, in this case, we
will generate duplicate ODS verification function.

Diff Detail

Event Timeline

Chia-hungDuan created this revision.Jun 16 2021, 4:47 AM
Chia-hungDuan requested review of this revision.Jun 16 2021, 4:47 AM

Nice, could you add a test? Any measurement of how much duplicate functions were being generated?

Brief update, there are 136 duplicates out of 712 verifiers.

Thanks for looking into this! Looks like a great improvement!

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

Can we use a DenseMap of SmallVector instead?

252

Can we just add a DenseMapInfo for Pred instead of adding the opaque access API?

jpienaar added inline comments.Jun 18 2021, 9:11 AM
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
233

Could you add a comment here too? I know you have above, but one here saying something like "Ensure the same error message is reported" or "Avoid merging verification what would produce different error messages". Although, come to think of it: in that case, if the same predicate would fail, then the failure is in order and we would not be able to trigger the one post (as this is per op), so do we even need to check this if it cannot be observed? Or is it observable?

Chia-hungDuan marked 2 inline comments as done.

Addressed review comments

Chia-hungDuan marked an inline comment as done.Jun 23 2021, 3:21 AM
Chia-hungDuan added inline comments.
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
233

When predicate fails we need to report the summary of the associated Constraint. Thus a predicate fail may happen under different contexts. For example, ConstraintA is applied on OpA, ConstraintB is applied on OpB and they have the same predicate. We need to report summary of ConstraintA if the predicate fails while verifying OpA, otherwise we need to report summary of ConstraintB.

So far I don't see this case, or we can give an error instead?

jpienaar accepted this revision.Jun 23 2021, 7:16 AM
jpienaar added inline comments.
mlir/include/mlir/TableGen/Predicate.h
17

OOC why is APInt needed here?

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

You are correct, I was thinking of predicates on same op vs different ones. And they could indeed be different (nothing stopping one predicate from having different high level info associated with it) so needs to be retained even if we don't do so today.

This revision is now accepted and ready to land.Jun 23 2021, 7:16 AM
Chia-hungDuan marked 2 inline comments as done.

Removed redundant header

jpienaar accepted this revision.Jun 29 2021, 9:52 AM
This revision was automatically updated to reflect the committed changes.