Improving parser/printer/verifier hooks

Over in this thread we added the ability in ODS for ops to state let hasCanonicalizeMethod = 1; to get a prototype for a wired up canonicalize method that you can go implement in any .cpp file you want. This works equivalent to how you specify let hasFolder = 1; and then go implement a fold method.

I think this approach is pretty nice for a variety of reasons, but it is also inconsistent with the parser/printer/verifier hook. Just to remember, these are specified like this:

let printer = [{ return ::print(p, *this); }];
let verifier = [{ return ::verify(*this); }];
let parser = [{ return ::parse$cppClass(parser, result); }];

These have annoyed me for some time for a couple of reasons:

  1. It leads to unnecessary “innovation” in implementation approaches for these hooks, which just makes the codebase more inconsistent.
  2. It leads to unnecessary c++ code in tblgen, and gives people the mistaken impression that they may want to put code inline.
  3. One of the standard approaches (shown above) depends on overloading of “::verify” and other methods, which leads to terrible error messages from clang when you mess something up, because the compiler spews the full overload set.

I think that going to a model of:

let hasCustomPrinter = 1;
let hasCustomVerifier = 1;
let hasCustomParser = 1;

would make sense, and then you implement a “well known method”, like folder and now canonicalizer do. Such an approach would be an incremental extension to the current model (so we can do a long slow transition over time). Does anyone have any objections or concerns about adding something like this?

-Chris

2 Likes

+1 on a standardized way to do this. The boilerplate and inconsistency of the current approach has always annoyed me. Can it be a static method on the class itself, as with the new canonicalize? That seems much nicer and more discoverable to me than mangling in the class name or overloading free functions, which I think we only have to do here because the user isn’t able to access the class declaration.

And now I’m thinking through how much cleanup we’ll have to do when integrating this… :smiley: I’d appreciate if we can keep the old way for a little bit to give downstreams a chance to adapt

Yes, that’s the idea (or instance method, in the case of print/verify). That allows you to implement the function in any file you want.

-Chris

1 Like

This has annoyed me a lot as well. Would be very happy to see this, and +1 to the idea of a standardized method for these (rather than standarized free function).

I think a reason for the innovation has also gone away with @mehdi_amini change to allow dialect specific behavior here. That reduces the need for custom that doesn’t use assembly syntax even more too, so these become even rarer (well at least printing and parsing, custom verification should also be reducing due to better modeling :slight_smile: ). I think you may run into issues with ops that have a print or parse attribute today and a named accessor is generated for, not sure how many of those there are.

FWIW, we are already generating methods for printer/parser/verifier, that’s why we have this in the inline code :). We just put the code blocks as method implementations and have no mechanism to only declare these methods letting the user provide the definition, hence the various forms of dispatch to static functions.

While I see some value in being able to specify short (1-2 lines) implementations inline, it creates problems for maintainability more often than it is useful. Especially with the declarative assembly and better trait-based verification that is now available. So it feels like a nice improvement.