Page MenuHomePhabricator

[mlir] Add OpAdaptorVerifier trait
Needs ReviewPublic

Authored by jpienaar on Feb 27 2021, 8:32 PM.

Details

Summary

Enables using OpAdaptor's verifier as trait. This allows for reordering
checking of OpAdaptor verification. Currently this verify method is
emitted in verify method via ODS which results in it being invoked last.
The basic verification of that the OpAdaptor does would be more
beneficial earlier and could rather always be verified first (e.g.,
automatically prepend OpAdaptorVerifier to traits in ODS) but this
change merely adds the trait pending discussion on alternatives to
achieve the above (which is also why its added only inside TestOps.td).

Diff Detail

Event Timeline

jpienaar created this revision.Feb 27 2021, 8:32 PM
jpienaar requested review of this revision.Feb 27 2021, 8:32 PM
bondhugula requested changes to this revision.Feb 27 2021, 8:51 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/include/mlir/IR/OpDefinition.h
306

I think this is not placed at the right point in this file and is also missing the banner comment. Below, you have: "Operand traits", "Result traits", "Misc traits", etc. Could you please put them under one of those or introduce a new one.

306

Could you please also expand the doc comment a bit to make it self-contained? Your commit summary actually has some information, but things aren't clear from this.

This revision now requires changes to proceed.Feb 27 2021, 8:51 PM
jpienaar updated this revision to Diff 328381.Mar 4 2021, 9:04 PM
jpienaar marked 2 inline comments as done.

Added heading and expanded comment

Is there more planned here? Seems like uses of this trait right now will result in double verification + double code size?

Is there more planned here? Seems like uses of this trait right now will result in double verification + double code size?

Correct (double verification, I don't think double code size as both calls the same verify method, but the compiler could inline). Next step would be to have ODS generate this as its first trait rather than invoking it in verify method generated. Now, this will result in many errors getting reported in different order and needing to update tests. I wanted to have this added/discussed in isolation before updating the tests (this only affects the one test op redundantly for now). And then the checking could be removed from type infrence methods using the adaptor verify manually today too (so that type inference can use attributes that are supposed to be specified).

The adaptor generated is also not as complete as it should be (it doesn't verify regions yet, nor does it verify as much of the operands as it could). So I'd want that more complete too.

bondhugula resigned from this revision.Mar 13 2021, 2:42 AM

Thanks. Please go with @rriddle's comments for the rest.