Derive macros for Printers and Parsers

[urso]

Hi,

I was looking for a project that allows me to represent my rather high level IR in SSA like form including support for printing and debugging the SSA. Think MLIR like and eventually found pliron. Also its still early in development the project looks interesting to me.

Although pliron allows me to store my IR, we don't get printers/parser for free yet. Unfortunately I'm not too lazy to write printers and parsers for my IR, yet I want them.

Long story short, I tried to implement proc macro to declare Ops, Types, Attributes, Parsers, and Printers in the hope that it makes my live (and maybe others) easier in the future.

It is still an early POC, but before I maybe waste too much time on it (or too much of your time by opening a huge cryptic PR) I would like to bounce of my ideas, make it a proposal and see if the project is interested in incorporating the changes I have in mind.
Also because I wonder if some of the code in my POC goes into a different direction as the rest of the code base does.

I'm happy to help making required changes to make it easier for users to create printers and parsers.

Proposal: derive Printable/Parsable from declarative assembly format

Note: Many of the things I'm proposing are available as code. Although I still might want to change a few things in my POC. You can check out the changes in the Diff.

When using MLIR it is possible to configure the printing and parsing of types, attributes, ops using the assemblyFormat setting. This setting provides a small MLIR specific DDL to configure how the current object shall be printed.
If no format is given some objects (e.g. Op) will use a generic format.

The idea is that we will provide a derive proc-macro for Printable and Parsable. Both of them accept a declarative assembly format that is compatible with the assemblyFormat setting used by MLIR.
In case the assembly format setting is missing we will fallback to a generic format for types/attributes/operations. This way users will get printers/parsers for free, but are able to refine the format with minimal effort.

Some examples how the configuration might look like (assuming we have Type, Attribute derive macros and so on):

Create integer type with generic format:

    #[derive(Hash, PartialEq, Eq, Debug, Type, Printable, Parsable)]
    #[type_name = "testing.integer"]
    pub struct IntegerType {
        sign: bool,
        width: u32,
        align: u32,
    }

The printer will create an output string like testing.integer<sign=true, width=32, align=8>.

Using asm_format setting we can configure the format explicitely. The generic format is equivalent to the follow piece of code:

    #[derive(Hash, PartialEq, Eq, Debug, Type, Printable, Parsable)]
    #[type_name = "testing.integer"]
    #[asm_format = "`<` `sign=` $sign `, width=` $width `, align=` $align `>`"]
    pub struct IntegerType {
        sign: bool,
        width: u32,
        align: u32,
    }

Similar to MLIR we also want to support directives. For example the function_type directive can be used like so:

    #[derive(Hash, PartialEq, Eq, Type, Printable, Parsable)]
    #[type_name = "testing.function"]
    #[asm_format = "function_type($inputs, $results)"]
    pub struct FunctionType {
        inputs: Vec<Ptr<TypeObj>>,
        results: Vec<Ptr<TypeObj>>,
    }

The use of the Printable/Parsable derive macros is optional. Users are still allowed to implement their own printers and parsers. One might also choose to generate the Printable parser only.

Next to the Parsable derive we might also provide a NotParsable derive macro for users that are not interested in that functionality.

Proposal: derive Op, Type, Attribute implementations

Note: For now I had implemented deriving the objects using proc_macros out of necessity. When deriving the printers/parsers we need to know what kind of IR object we are going to have. The trick I'm using is to check the macro attributes for type_name, attribute_name, or ir_kind settings. a pure proc macro "implementing" an operation can splice in the #[ir_kind="op"] into the token stream and this way give the Parsable/Printable derive macros some additional information.

Operations always create a struct like struct MyOp { op: Ptr<Operation> }. The current declare_op create the struct for our users.
Using proc macros an Op shall be declared like so:

    #[declare_op]
    #[op_name = "testing.myop"]
    #[derive(Printable)]
    struct MyOp;
    impl Verify for MyOp {
        fn verify(&self, _ctx: &Context) -> Result<()> {
            Ok(())
        }
    }

The declare_op macro will rewrite the struct MyOp to struct MyOp{ op: Ptr<Operation> } and add the #[ir_kind="op"] attribute for other derive macros to pick up. The template for declare_op reads similar to this:

fn impl_struct(input: Struct) -> TokenStream {
    let name = &input.ident;
    let verifiers_name = format_ident!("OpInterfaceVerifier_{}", name);
    let dialect = input.attrs.dialect;
    let op_name = input.attrs.op_name;
    let kind = input.attrs.kind;
    let attributes = input.attrs.other;
    quote! {
        #[derive(Clone, Copy)]
        #(#attributes)*
        #kind
        pub struct #name { op: ::pliron::context::Ptr<::pliron::operation::Operation> }

        #[allow(non_camel_case_types)]
        pub struct #verifiers_name(pub ::pliron::op::OpInterfaceVerifier);

        impl #name {
            pub const fn build_interface_verifier(verifier: ::pliron::op::OpInterfaceVerifier) -> #verifiers_name {
                #verifiers_name(verifier)
            }
        }

        inventory::collect!(#verifiers_name);

        impl ::pliron::op::Op for #name { ... }
    }
}

Similar to declare_op will we support a declare_type and a declare_attribute macro

Note: in TableGen one uses def Name, maybe we want to rename declare_X to def_x?.

A type can be declared like so:

    #[declare_type]
    #[type_name = "testing.integer"]
    #[derive(Hash, PartialEq, Eq, Debug,  Printable, Parsable)]
    pub struct IntegerType {
        sign: bool,
        width: u32,
        align: u32,
    }

Attribute example:

    #[declare_attribute]
    #[attr_name = "testing.integer"]
    #[derive(PartialEq, Eq, Clone, Printable, NotParsableAttribute)]
    #[asm_format = "` <` format(`0x{:x}`, $val) `: ` $ty `>`"]
    pub struct IntegerAttr {
        ty: Ptr<TypeObj>,
        val: ApInt,
    }

Note: The POC currently uses an Attribute and Type derive macro. The limitation is that the Parsable/Printable derivers must understand the settings of the other macros to implement some heuristics on what kind of object they are dealing with. Having declare macros allows us to splice in a common #[ir_kind="..."] attribute. Plus it feels much more consistent with the declare_op macro.

POC: Asm format implementation/backend

Feel free to ignore this section if you are not interested in the details on how printing/parsing helpers are implemented in the POC.
This section is more about giving you hints and an explanation to the changes I made to the project to get this working.

The Printable/Parsable derive macros parse the asm_format attribute and generate some code based on that definition.

We want to keep the code generated minimal. Which is why we provide all functionality via the asmfmt module.

The asmfmt module provides printers and parsers (and combinators) which allows users to reuse the primitives used by asmfmt in custom code.

The POC has no common objects that support both printing and parsing. Instead we have a set of functions with similar names in each module. For example parsers::literal and printers::literal. This is maybe not ideal, but allows us to play with the interfaces until the code (and interfaces) have stabilized a little. e.g. within the POC parsing is not yet really implemented at all.
Later for common object types we might consider to introduce structs that both implement the printer and parser interfaces.

Directives are implemented via macro_rules. Originally I started with functions for the directives, but hit some limitations dealing with fields that might implement Printable, Display, or both. By using macros we give the code some flexibility to handle different cases. Directives always have the suffix _directive.

For example the function_type directive:

#[allow(unused_macros)]
macro_rules! function_type_directive {
    ($ty:expr, $inputs:expr, $results:expr) => {
        function_type(print_var!(&$inputs), print_var!(&$results))
    };
}
#[allow(unused_imports)]
pub(crate) use function_type_directive;

pub fn function_type<'a>(
    inputs: impl Printable + 'a,
    results: impl Printable + 'a,
) -> impl Printable + 'a {
    ...
}

Here we use the print_var macro that uses autoref specialization to give us a printer for types that implement Printable, or a collection of Printables, or types that only implement Display.

Printers

The core concept of the asmfmt::printers module is that we have a composable set of printers.

A printer (generator) is a function that returns a impl Printable. Currently the code uses a more or less functional approach. We use PrinterFn as wrapper to create our set of printers. For example the literal printer can be implemented like so:

pub fn literal(lit: &str) -> impl Printable + '_ {
    PrinterFn(
        move |_ctx: &Context, _state: &State, f: &mut fmt::Formatter<'_>| write!(f, "{}", disp),
    )
}

We also provide some combinators like concat, list_with_sep, or iter_with_sep.

The list_with_sep and iter_with_sep combinator replace the PrintableList interface, which has actually been removed. I did struggle integrating PrintableList into the set of combinator and found it easier to maintain composability of printers by relying on a single interface only. In my mind I found the Printable is very capable to deal with all our needs.

Using the combinators the operators directive in MLIR assemblyFormat can be implemented like this:

fn operands(ops: &[Operand<Value>]) -> impl Printable + '_ {
    let sep = ListSeparator::Char(',');
    concat(("(", list_with_sep(ops, sep), ")"))
}

The attr-dict directive primitive becomes:

fn attr_dict(attrs: &AttributeDict) -> impl Printable + '_ {
    PrinterFn(
        move |ctx: &Context, state: &State, f: &mut fmt::Formatter<'_>| {
            let attrs = attrs.iter()
                .map(|(k, v)| concat((quoted(k), literal(" = "), v)));
            let sep = ListSeparator::Char(',');
            concat(("{", iter_with_sep(attrs, sep), "}")).fmt(ctx, state, f)?;
        },
    )
}

By providing all assembly format primitives as combinators we allow users to writing custom printers reusing most of the primitives that we have already.

For example MLIR generic op format implementation used by the Printable derive macro as fallback.

Parsers

Honestly, I really struggled with the parser interface. The combine::Parser as is was hard to work with. It seems to be doable by using some of the combine parser combinators, but the error returns are a really difficult to work with.

The Parsable trait makes it easier to handle errors and gives us all the state that we are interested in, but unfortunately it is difficult to declare reusable primitives similar to the asmfmt::printers. Especially because Parsable does not really allow me to pass a self parameter into the parser. I tried to trick Arg to do that for me, but the code did become quite painful.
I also struggled with the bound 'a in fn parse<'a>. Not having the bound on the trait did make my approach prototyping parsers via Fn fail.

As a workaround to have a functioning parser I introduced yet another parser trait :(

impl<'a> AsmParser<'a, &'a str> for &'static str {
    fn parse_next(&self, state_stream: &mut StateStream<'a>) -> ParseResult<'a, &'a str> {
        literal(self).parse_next(state_stream)
    }
}

This is definitely not ideal as we now have 3 traits in the system that tell us on how to declare and compose a parser. I halso had to name the method parse_next because the compiler didn't like me mixing all the traits in a single consuming module :(

The parsers module provides a from_parseable wrapper to create a parser from a Parsable type, so we can mix the two.

Based on that we can prototype the literal parser like so:

pub fn literal<'a>(lit: &'static str) -> impl AsmParser<'a, &'a str> {
    move |state_stream: &mut StateStream<'a>| {
        spaces().parse_stream(state_stream).into_result()?;
        let x = string(lit).parse_stream(state_stream).into_result()?.0;
        Ok((x, Commit::Commit(())))
    }
}

For example in the tests (Note: I should put them into its own file). We have an a custom IntegerType without asm_format like:

#[derive(Hash, PartialEq, Eq, Debug, Type, Printable, Parsable)]
    #[type_name = "testing.integer"]
    pub struct IntegerType {
        sign: bool,
        width: u32,
        align: u32,
    }

The derive macro creates this Parsable implementation (currently all generated code is printed to console when compiling):

    impl Parsable for IntegerType {
        type Arg = ();
        type Parsed = Ptr<TypeObj>;

        fn parse<'a>(
            state_stream: &mut StateStream<'a>,
            _arg: Self::Arg,
        ) -> ParseResult<'a, Self::Parsed> {
            literal("<").parse_next(state_stream)?;
            literal("sign=").parse_next(state_stream)?;
            let sign = from_parseable().parse_next(state_stream)?.0;
            literal(", ").parse_next(state_stream)?;
            literal("width=").parse_next(state_stream)?;
            let width = from_parseable().parse_next(state_stream)?.0;
            literal(", ").parse_next(state_stream)?;
            literal("align=").parse_next(state_stream)?;
            let align = from_parseable().parse_next(state_stream)?.0;
            literal(">").parse_next(state_stream)?;
            Ok((
                ::pliron::r#type::Type::register_instance(
                    Self { sign, align, width },
                    state_stream.state.ctx,
                ),
                Commit::Commit(()),
            ))
        }
    }

POC: Proc macro implementations

Feel free to ignore this section if you are not interested in the details on how printing/parsing helpers are implemented in the POC.
This section is just an intro to the derive macro implementations to make it easier to get to know the changes.

You can find the derive macros in the derive folder. All macros follow the same patterns of: parse input, verify input, produce output from template. All derive implementations return a syn::Result which is translated into a compiler error if verification/parsing failed.

derive/src/asmfmt.rs Implements the parsing of the assembly format DSL, producing a common representation to be used by the Printable/Parsable derive macros.

Note: As I struggled with combine quite a bit I opted to use winnow here.

plirons Crate.toml was updated to import pliron_derive. Plus src/lib.rs was update such that pliron can be used as a modue name within pliron (this allows us to use the derive macros in pliron itself):

// Allow proc-macros to find this crate
extern crate self as pliron;

Because the assembly format directives are implemented as macro_rules we ensure that the macros can be found within the pliron project by declaring the internal macros like so:

#[macro_export]
macro_rules! <macro_name> {
  ...
}
#[allow(unused_imports)]
pub(crate) use <macro_name>;

Follow ups?

derive Verify

This proposal focuses on Printer/Parsable. But once the machinery is in place we might consider a derive macro for Verify as well.

Indentation support?

The indentation support seems to require us to explicitly use helpers to create properly indented new lines. When using asm_format with a \n literal that might break the output and indentation level.

Maybe we have to parse and handle that case in the literal helper. Parse the string for newlines and append the proper set of spaces after a newline.

For formatting/output I wonder if you have considered pretty? Although not sure if we really want to build an intermediate "document" representation before formatting the output.

I recently have had a similar problem and did find the indent-display[https://docs.rs/indent-display/0.1.1/indent_display/#traits] cargo quite interesting. That one splits the lines on \n and applies the proper indentation. This mixes somewhat well with types that implement Display and still want to produce a multiline output. Plus "pop_indent" is implicitly provided via RAII.

Just some thoughts. I'm thinking to add the newline + indentation support to asmfmt::literal only.

Parsers?

Honestly working with combine + the Parsable trait and the different Result return types (some supporting Try, others not...) was a big pain and caused me much headaches. It also seems like loads of boxing is going on here.

This is what forced me to introduce the AsmParser trait, just to make some progress and pin down the types.

I don't have much experience with combine yet, but honestly I find the parser trait of winnow to be much easier to work with. In the derive macro I started with combine, but got frustrated with it and switch over.

I also wonder if it would make sense to introduce a lexer and have the parser operate on tokens. Having a lexer would trim down the lexical grammar limiting what can be done in parsers (and you don't have to think about spaces). MLIR for example follows that approach. Plus they have a list of simple parsers to be combined, which is not too dissimilar from the approach used in the asmfmt module.

[vaivaswatha]

Thank you @urso for this proposal. It's interesting, and overall useful. I need to read it again in more detail and digest some of it, but overall, I'm in favour of what you're doing.

I'll just address some points (which came to my mind immediately) here and write another response after understanding it all in more detail.

1. I'm not sure we need this for Op, because all we need is a print_op_canonical_format function that does it, and similarly a parser. I've been meaning to add one, but haven't yet. Someone wanting a custom Op syntax can impl Printable and Parsable. But having said that, it is still good to have it for Op also, since your macros seem to make it easier to define custom formats too.

2. For formatting/output I wonder if you have considered pretty? Although not sure if we really want to build an intermediate "document" representation before formatting the output.

   Yes I did look at it. I didn't like the idea of that "doc" representation as an intermediate.

This is definitely not ideal as we now have 3 traits in the system that tell us on how to declare and compose a parser. I halso had to name the method parse_next because the compiler didn't like me mixing all the traits in a single consuming module :(
The parsers module provides a from_parseable wrapper to create a parser from a Parsable type, so we can mix the two.

I think we can figure out a way to merge your trait into the existing one and make it all more neat.

I recently have had a similar problem and did find the indent-display[https://docs.rs/indent-display/0.1.1/indent_display/#traits] cargo quite interesting. That one splits the lines on \n and applies the proper indentation. This mixes somewhat well with types that implement Display and still want to produce a multiline output. Plus "pop_indent" is implicitly provided via RAII.

I'd used it in an earlier version of pliron, but decided against it as it's too inefficient (at ever level, we'll end up rewriting the whole string).

but the error returns are a really difficult to work with.

I'm happy that I'm not the only one with that opinion :) .. I'm, even now, working on making it easier. And that's also the reason I suggest that I need a little more time before I can confidently request for (merge) your derives for Parsables.

Overall, thank you again for what you're doing. Let's discuss more and merge it all soon, at least the Printable parts. We can decide on the Parsable part in a couple days depending on how my attempt at making the interface smoother goes.

The derive macro creates this Parsable implementation (currently all generated code is printed to console when compiling):

It looks really cool :-)

but unfortunately it is difficult to declare reusable primitives similar to the asmfmt::printers. Especially because Parsable does not really allow me to pass a self parameter into the parser.

Can you elaborate more? My whole idea was to have them be reusable. For example, a Region parser uses BasicBlock::parser() as a combinator and so on. i.e., they're easily composible. Why does having a self parameter help? Maybe with a specific example, I can see if it can be made to work.

[urso]

I'm not sure we need this for Op, because all we need is a print_op_canonical_format function that does it, and similarly a parser. I've been meaning to add one, but haven't. Someone wanting a custom Op syntax can impl Printable and Parsable. But haven't said that, it is still good to have it for Op also, since your macros seem to make it easier to define custom formats too.

+1 supporting the generic format out of the box. At least it gets you started with your own IR very easily. But the generic format is also super verbose.

It would still be nice to use the Printable and Parsable derives. That way it looks consistent to support the derives for all of the object types + it reduces the code users have to write. For the time being we could disallow the asm_format attribute for Ops and always use the generic format.

What I have had in mind of for Op as e.g. the scf or affine dialect.
Checking MLIR code they only use assemblyFormat for the simpler op types though (for example condition op).

For formatting/output I wonder if you have considered pretty?

Yes I did look at it. I didn't like the idea of that "doc" representation as an intermediate.

Yeah, I thought so.

I recently have had a similar problem and did find the indent-display

I'd used it in an earlier version of pliron, but decided against it as it's too inefficient (at ever level, we'll end up rewriting the whole string).

Fair. I think I will handle this in the asmfmt parser in the derive macro then. I also wanted to split literals on whitespace and maybe some symbolic characters, such that input like ", abc=" will be translated into 3 literals: ,, abc, =. The split helps with the parser as hand written input might use whitespace a little more liberally.

I'm happy that I'm not the only one with that opinion :) .. I'm, even now, working on making it easier. And that's also the reason I suggest that I need a little more time before I can confidently request for (merge) your derives for Parsables.

I'm glad to see improvements in that area :)

Can you elaborate more? My whole idea was to have them be reusable. For example, a Region parser uses BasicBlock::parser() as a combinator and so on. i.e., they're easily composible. Why does having a self parameter help? Maybe with a specific example, I can see if it can be made to work.

Don't get me wrong. I still think we need something like Parsable to some extend. But trait wise what we need feels to be more in the class of FromStr, From, or TryFrom traits. That is it kinda acts as a 'Constructor'-like entity.

In my mind Parsers and parser combinators on the other hand carry some configuration so to say. This is true for the combine::Parser trait, but also the AsmParser trait that I introduced. The methods of the traits all have a self argument. This is what allows me to implement helpers like literal, build combinators like list_with_sep, or wrap a type implementing Parsable into a parser without additional boxing. The combinator function captures the other parsers as its configuration and reuses that configuration when parsing the actual input stream.

I initially tried using Parsable only, for example for literal I did try something like this:

struct Literal(&'static str);

impl Parsable for &Literal {
  type Arg = Self;
  type Parsed = &'a str;  // <- this is not possible, because the bound `'a` is on parse, but not the trait. I had to use `String` or `()`

  fn parse<'a>(state_stream: ..., arg: Self::Arg) ... {
    combine::string(arg.0).parse_stream(state_stream).into_result()?
  }
}

I later opted to introduce AsmParser as this was not very ergonomic to use.
Using AsmParser this becomes:

impl<'a> AsmParser<'a, &'a str> {
  fn parse_next<'a>(&self, state_stream: ...) -> ParseResult<'a, &'a str> {
      combine::strings(self.0).parse_stream(state_stream).into_result()
  }
}

Still having Parsable is a good thing to have. The generated parser uses this construct for variables/attributes/types to be parsed:

let field_name = from_parsable::<T>().parse_next(...)?;

The from_parsable combinator integrates Parsable and other traits into the system, such that we can have custom syntax and type-directed parsing of internal variables.
I assume we also need Parsable because we need to store the mapping of object-id -> parser somewhere in order to selectively use the correct parser for attributes.

Having from_parsable helper I also wonder if we can remove the parser and parser_fn methods from the Parsable trait. In case you want a boxed parser from a Parsable just use from_parsable::<MyType>().boxed(). from_parsable does not allocate anything as it just wraps the contained type.

I think from_parsable and Parsable traits is a must. This is because in macros we can not inspect the target type of structs field reliably. We need to rely on traits and rust compiler finding us the correct implementation.

But this also makes me wonder if AsmParser would be a good fit as a default parser interface within the project. The trait is much simplified and one can use combine::Parser, AsmParser, and Parsable in AsmParser based parsers function bodies. Plus similar to MLIR we could provide elemental parsers as functions as is.

Assuming AsmParser is the default parser interface I wonder if Parsable could be replaced with something like this:

trait FromAsm<'a, T=Self> {
  fn from_asm(state_stream: &mut StateStream<'a>) -> ParseResult<'a, T>
}

or making use of the new return position impl trait feature (in stable with rust 1.75):

trait Parsable<T> {
  fn parser<'a>() -> impl AsmParser<'a, T> // <- no enforced boxing/allocation yet
}

For types we would require users to implement Parsable<Ptr<TypeObj>>.

That trait works well for types and attributes as is. For types and attributes it is a must that this interface is implemented. This is what allows us to integrate those types with the code generated from the macros + register the parsers in a shared table mapping object id names to parsers.

For Operations we currently require a parser that also accepts a Vec<Identifiers> as object. I'm not deep enough in the code to know if we really need Vec<Identifier> as inputs, or if we can have the FromAsm return just the Operation. I only noticed the argument to the op to be used as validation (check number of IDs) or in case of the constant op to register something with the name tracker.
When implementing new parsers for ops it would be helpful if downstream users do not have to know on how to interact/register the parserd object with the other pliron internal structures. Let users focus on parsing the "body" and provide meta-data for pliron to do the right thing if we need so.
It seems that it would even be possible to verify results and maybe register something in the name-tracker based on the internal Operation object returned by the parser. Maybe have a parse_op asm parser function that uses the trait to parse the body and verifies + registers the result with the name tracker after the body has been parsed?

The other exception is Parsable for Region. But Region::parser_fn is not used. Using the asmfmt approach we might want to turn this one into a simple parse_region<'a>(op: Ptr<Operation>) -> impl AsmParser<'a, Region> function or similar. To support the assemblyFormat region directive we might need this function anyways.

The good thing about MLIR is that although the IR is open and extensible, the format of common object types like Types, Attributes, Ops, Regions is still limited. This reduces the need for a fully generalized parsing approach. A set of 'smaller' but reusable parsing functions with concrete types might be good enough. At least this is the approach I've chosen for the printers.

[vaivaswatha]

For Operations we currently require a parser that also accepts a Vec as object.

When implementing new parsers for ops it would be helpful if downstream users do not have to know on how to interact/register the parserd object with the other pliron internal structures. Let users focus on parsing the "body" and provide meta-data for pliron to do the right thing if we need so.

These two are related. The way to parse any operation is to call Operation::parser() (provided by the framework). What it does is look for either of the pattern

res1, ...., resn = opid
OR
opid

and pass on the control to the Op's registered parser. For a canonical syntax, I'll provide a function that the Op's parser can call (this is a TODO still), or the Op can choose to define it's own custom syntax parser (this is what currently defined Ops are doing). So users will focus on the "body" and do not have to deal with fetching the registered parser etc.

and for this, above, to work, the outer Operation parser, which has already looked at the results will provide it, as is, as parameters to the Op's registered parser, to do whatever it wants with it.

Regarding your suggestions and questions on the AsmParser you introduced, let me play around with your code a little bit and get back.

[vaivaswatha]

I pushed a couple of simplifications / improvements (IMO) to Parsable. (one of it - an alias for StdParseResult2 was taken from your branch).

When you get time, may I ask that you update your branch so that I can play around with AsmParser and see how it can be made to best co-exist with Parsable ?

Thank you again.

[urso]

When you get time, may I ask that you update your branch so that I can play around with AsmParser and see how it can be made to best co-exist with Parsable ?

Sure. I just rebased the branch on top of your latest changes (Diff)

[urso]

and for this, above, to work, the outer Operation parser, which has already looked at the results will provide it, as is, as parameters to the Op's registered parser, to do whatever it wants with it.

I see. This is the part I wonder about. My impression is that this way the Op parser has to participate in building the SSA. Normally I would assume parsing operates like this:

1. parse lhs (the result IDs)
2. parse assignment `=`
3. parse rhs (the operation)
4. register lhs with rhs

step 4 would optionally validate that lhs and rhs are compatible, but IR validation could also be postponed to a second stage.
By not giving control to the op parser we limit the scope of what we require the op parser to do. And also reduce the knowledge downstream developers need to have about pliron.

Checking the MLIR sources, this is exactly how the MLIR AsmParser operates:

1. parse the the resultIDs until = (including the = symbol)
2. use the Op parser to get an op.
3. do some checks and register the result IDs and Op

The way MLIR does it is pretty much what I have in mind. Have the op parser concentrate on parsing the body only. I don't think we should have downstream developers help in building the IR. All the name tracking and instruction building (adding ops and assigning ops to the IR) should be internal to pliron.

Another thing that I like about the MLIR approach is that the MLIR parseOperation chooses if it wants to use the generic format or call the op-parser depending on the first token for the Op use. This way MLIR can guarantee that the generic format always works even if an Op implements a custom parser. The reason I like this is that it keeps printed output potentially compatible with future IR updates that introduce a custom parser.

Due to MLIR always guaranteeing they support the generic format (instead of pushing that decision on the Op-developer) it would allow users to eventually convert a file to the generic format in case of breaking changes in the ASM format of an op. Then I as user can take my file (that I use in CI), print the generic format using the old code, read the generic format and print the new format using the new code. Maybe not as big of an issue in general, but for me as developer putting instruction files into the repo for CI it is a nice feature to have.

[vaivaswatha]

parse the the resultIDs until = (including the = symbol)
use the Op parser to get an op.
do some checks and register the result IDs and Op

We could do this, but the interface would prevent us from having Ops which can have different instances with different number of results. The Op parser, which creates the actual Operation object wouldn't know how many results to allocate. One way to overcome that is to provide functions to modify the results of an already created Operation, but I somehow think we shouldn't do that. Even MLIR doesn't allow that.

MLIR doesn't have this problem (i.e., they can parse such Ops that can have different number of results) because their Op parser doesn't actually allocate an Operation. They use an intermediate OperationState object. What we have seems neater to me (with the exception below - next paragraph).

I agree about your concern that users shouldn't be dealing with registering / checking SSA names. The problem is easily solvable for operands by providing an Operand::parser() that looks for an identifier and does the checking the registry for SSA use. For definitions (results) it wouldn't be so easy, but let me think about it a bit.

Another thing that I like about the MLIR approach is that the MLIR parseOperation chooses if it wants to use the generic format or call the op-parser depending on the first token for the Op use

From what I understand, what we have is strictly more general than this. an Op::parser can choose to delegate back to a generic parser format, or define it's own custom format, or do an Or of them both, supporting both. We could provide utilities to make it easier, but everything you ask for is / can be there. Am I missing something?

In summary, it appears to me that having the arg parameter on Parsable::parse is the right way to go. The main problem with it is it's making the parser generators / derivators that you're writing difficult (am I correct in this inference?). Let me look at your branch in detail next and see if something can be down about this all. I'll keep in touch on this thread if I have any questions / suggestions. We'll take a call after that.

[vaivaswatha]

I agree about your concern that users shouldn't be dealing with registering / checking SSA names.

This commit tries to address the problem. It isn't 100% satisfactory, but it is nearly there.

[urso]

MLIR doesn't have this problem (i.e., they can parse such Ops that can have different number of results) because their Op parser doesn't actually allocate an Operation. They use an intermediate OperationState object. What we have seems neater to me (with the exception below - next paragraph).

I see. I wasn't aware that Op parsing in MLIR is not context free. Alternatively to requiring Parsable to have type Arg one could introduce an OpParser trait like so:

pub trait OpParser<'a> {
  fn parse(results: &[Identifier], state_stream: &mut StateStream<'a> ) -> ParseResult<'a, Self> {
      ...
  }
}

It seems like it doesn't really matter if users implement a trait on their Op as we do register a parser function with the dialect.

But in the end it doesn't really matter if we have the generic Parsable trait that accepts Args or more type directed traits like OpParser. Either one can easily be into the asm parser.

It seems that the Op parsing will only be driven by Operation::parse only.

This commit tries to address the problem. It isn't 100% satisfactory, but it is nearly there.

Nice.
We will need the ssa_opd_parse for the operators asm format directive I think.

The process_parsed_ssa_defs function is what I wonder about. Why can't we always call it from Operation::parse? Maybe we wold have to turn the assert into an error in that case. This would allow downstream developers to not pay attention about the result IDs if they choose to use a context free approach and rely on the framework for checking correctness.

In summary, it appears to me that having the arg parameter on Parsable::parse is the right way to go. The main problem with it is it's making the parser generators / derivators that you're writing difficult (am I correct in this inference?). Let me look at your branch in detail next and see if something can be down about this all. I'll keep in touch on this thread if I have any questions / suggestions. We'll take a call after that.

Well, it is easy to integrate and use Parsable. Especially if Args = (). And for Ops/Regions we will introduce a directive/wrapper calling Region::parse. As only regions/Ops parsing is not context free that should not cause any friction.

It is just the for the actual parser functions/directives I added that the Parsable trait was difficult for me to work with. Plus there is loads of boxing going on if one wants a parser_fn for reusing the parser with combine it seems.

[vaivaswatha]

The process_parsed_ssa_defs function is what I wonder about. Why can't we always call it from Operation::parse?

That would be ideal. At the moment I don't because the current syntax allows for an Op's result to be in the "custom syntax" part of it. i.e., theoretically we could have an Op with syntax opid res1, res2 = ... , and that is what brings up the scenario where it'll have to be the custom parser that calls the process_parsed_ssa_defs function.

One solution could be to have the custom parser return any other results (what weren't passed to it when it was called) back so that we an merge them all and call this function. That doesn't seem nice at all. The other solution would be to restrict that any results must be parsable by Operation::parser, i.e, they must all come before the =. I don't know if that restriction is too much or not at the moment. If we do that, then we can do what you suggested.

Plus there is loads of boxing going on if one wants a parser_fn for reusing the parser with combine it seems.

I didn't understand this. Can you point me to a place in your code to illustrate? Maybe that's where I can begin looking at your code, to get started. All Parsable objects have a parser() method, which if you call, you get a parser that can be combined with anything from combine. For other parsers, such as type_parse (a utility to parse typeid, followed by calling the type's custom parser), there is a corresponding type_parser() that will create a parser for use with combine.

[urso]

At the moment I don't because the current syntax allows for an Op's result to be in the "custom syntax" part of it. i.e., theoretically we could have an Op with syntax opid res1, res2 = ... , and that is what brings up the scenario where it'll have to be the custom parser that calls the process_parsed_ssa_defs function.

I see. As a consequence of that I introduced a results asm format directive to the printers and made Vec<OpResult printable.

When using the asm format configuration we will basically prepend (results^ =)? to the printers. Custom printers will be free to choose how to print the results.

The discussions about Parsable got me thinking and I will spend some time to overhaul the Printers a little. In the end I would like to end up with uniform declarative asm format syntax that will work for both, parsers and printers.

Plus there is loads of boxing going on if one wants a parser_fn for reusing the parser with combine it seems.

I didn't understand this. Can you point me to a place in your code to illustrate? ...
All Parsable objects have a parser() method, which if you call, you get a parser that can be combined with anything from combine

By boxing I don't mean that I need to wrap anything. It is the parser and parser_fn methods on Parsable that return a Box<dyn Parser>. With boxing I mean the use of Box, which results in temporary allocations in order to erase the actual type. It's maybe not too bad, just a minor annoyance.

Maybe that's where I can begin looking at your code, to get started.

Regarding parsing there is not much code. There are just a few test parsers in src/asmfmt/parsers.rs. I'm still mostly focusing on the printers and printer directives. The tests module in asmfmt::printers tries to do some parsing for SimpleType and there is a parse_integer_type test function (yeah, I need to move them somewhere else)

[vaivaswatha]

As a consequence of that I introduced a results asm format directive to the printers and made Vec<OpResult printable.

I suppose you mean this ?

The code change goes against what you're saying. If we do that, then custom printers will be forced to have their results always printed ahead of the opid. self.results.fmt(...) shouldn't be called from Operation::fmt but from a print_operation_canonical_syntax or some such utility function that will print the entire Operation in a canonical syntax. This is the function I told that I need to add (I think I saw something to that effect in your branch already) and which custom printers can call if they want to just resort to having the canonical syntax for itself.

A few more comments on your branch

1. The return here isn't needed. I removed them all recently. The ? takes care of it.
2. Do you think we can do separate PRs for unrelated changes, such as the Typed trait, removal of unnecessary &mut for ctx parameters, AttributeDict separation etc.
3. I'm a bit hesitant about changes such as this. While they do look good, they may not be as intuitive as using a format! like syntax which most Rust programmers would already know. As long as both methods work, it should be ok I think.

[urso]

I suppose you mean this ?

The code change goes against what you're saying. If we do that, then custom printers will be forced to have their results always printed ahead of the opid. self.results.fmt(...) shouldn't be called from Operation::fmt but from a print_operation_canonical_syntax or some such utility function that will print the entire Operation in a canonical syntax.

Oops, I didn't intend to commit this. I indeed removed it from Operation::fmt already when I pushed that commit.

Do you think we can do separate PRs for unrelated changes, such as the Typed trait, removal of unnecessary &mut for ctx parameters, AttributeDict separation etc.

Absolutely. It is still very much work in progress. But I will definitely split the PR up in smaller pieces.

I'm a bit hesitant about changes such as this. While they do look good, they may not be as intuitive as using a format! like syntax which most Rust programmers would already know. As long as both methods work, it should be ok I think.

I understand. I actually didn't mean to keep them. It was me testing if the helper function type check and work correctly without changing all the existing Ops to use the derive macros.

In case you want to discuss on the code I did open a draft PR: #20

Actually I opened it against my own local fork, but for some reason github decided to create the PR against your repository. Sorry for that.

I'm planning to eventually split the PR into smaller PRs once it stabilizes a little more.

[vaivaswatha]

Absolutely. It is still very much work in progress. But I will definitely split the PR up in smaller pieces.

Thank you. I didn't do a detailed review, but I like the direction in which the whole thing is going. So can I pause for the moment now and just wait until you ping me back, saying that it's ready for review? Is there anything specific that you want me to comment on or help you with now?

[urso]

I think we can pause the conversation for now. I will focus on the printers and maybe ask you for guidance when I'm about to split up the PR.

[urso]

Hi,

besides me adding more tests I'm happy with the proc_macros for def_attribute, def_op, def_type and the Printable derive macro for now.

I started to split the work into PRs. Let's start with the new traits I introduced: #21

[vaivaswatha]

This mostly works now, see: https://github.com/vaivaswatha/pliron/wiki/Declarative-Syntax-Specification. So closing.