Page MenuHomePhabricator

[mlir] Add VerifyOpBeforeTraits trait
Changes PlannedPublic

Authored by springerm on Nov 16 2022, 9:12 AM.

Details

Summary

Add a new "marker" trait that allows users to specify the order in which ops are verified.

By default: verify traits, then the op
When VerifyOpBeforeTraits is attached: verify op, then the traits

VerifyOpBeforeTraits is useful when an interface verifier calls an InterfaceMethod. Without VerifyOpBeforeTraits, the op was not verified yet (and could be broken), so calling an InterfaceMethod is dangerous and could crash.

Diff Detail

Event Timeline

springerm created this revision.Nov 16 2022, 9:12 AM
springerm requested review of this revision.Nov 16 2022, 9:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 16 2022, 9:12 AM

Very nice, this solves some very concrete problems!

LGTM but please wait for @rriddle and @mehdi_amini to weigh in.

This revision is now accepted and ready to land.Nov 16 2022, 10:01 AM
rriddle requested changes to this revision.Nov 16 2022, 11:44 AM

This doesn't really feel like the right direction. It's really fragile to flip verification either before or after the operation. We often have "core" traits that we expect to be verified before we ever get to the operations "verify" method. A better path forward would be evolving the trait verification model, which there have been discussions of in the past on discourse (can't find the thread right now).

This revision now requires changes to proceed.Nov 16 2022, 11:44 AM

Agree with River: this seems like a workaround to me.
Here is the previous thinking about this: https://discourse.llvm.org/t/rfc-verification-order-in-mlir/4718 ; can you position your issue with respect to this?

springerm planned changes to this revision.Thu, Jan 12, 12:32 AM