[PATCH] D80651: Harden MLIR detection of misconfiguration when missing dialect registration

Wed May 27 16:23:29 PDT 2020

rriddle added a comment.

In D80651#2058617 <https://reviews.llvm.org/D80651#2058617>, @jpienaar wrote:

> In D80651#2058607 <https://reviews.llvm.org/D80651#2058607>, @mehdi_amini wrote:
>
> > In D80651#2058483 <https://reviews.llvm.org/D80651#2058483>, @rriddle wrote:
> >
> > > I wonder if we should also add something to Op::classof. i.e. can you cast to FooOp if it isn't registered?
> >
> >
> > Right now I was trying to add code that runs in release mode, so I tried to hook into code path that are OK to pay the price: type creation is once per type and op creation is expensive enough to take a nullptr check.
> >  Are you proposing to have an extra nullptr check for every cast to an operation? I can also do that.
>
>
> I'd prefer if we do keep it light: it would seem that if one is testing this with unit tests/NDEBUG then these would be caught with asserts . So with proper unit tests one should be catching these & should be rare (e.g., how often is a new dialect added to production build?) [which is also why I'm a little bit torn but don't feel strongly against].

I brought this up because if we have a policy of not allowing the use of *Op classes for unregistered ops we should be consistent with erroring out.

As for enabling in release builds it depends on how often we expect unregistered operations to be used in production. If it is often, then we should only enable during debug builds. If not, it is fairly cheap to enable during release given that `classof` already has a fast path for operations that are registered. Regardless of which one we choose, it has the benefit of removing the fallback string comparison in favor of always returning false.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80651/new/

https://reviews.llvm.org/D80651