Page MenuHomePhabricator

[PDLL] Add proper expansive documentation for PDLL
ClosedPublic

Authored by rriddle on Feb 15 2022, 3:43 PM.

Details

Summary

This commit adds detailed documentation for PDLL, its language design, and
captures a bit of the rationale. This document captures everything in-tree at present,
and is intended to be an all encompassing manual for interacting with and understanding
PDLL.

Diff Detail

Event Timeline

rriddle created this revision.Feb 15 2022, 3:43 PM
rriddle requested review of this revision.Feb 15 2022, 3:43 PM
zero9178 added inline comments.
mlir/docs/PDLL.md
689

I think it would be nice if either in a comment right about here, or maybe in the sentence above the code block, it'd be mentioned that resultOp.result and resultOp.0 refer to the exact same result. This makes it clear that this is done as a demonstration and there is no actual semantic difference when using one form over the other.

1183

Should this be CreateRewriteOps?

rriddle updated this revision to Diff 409104.Feb 15 2022, 5:18 PM
rriddle edited the summary of this revision. (Show Details)
rriddle marked 2 inline comments as done.
mlir/docs/PDLL.md
325

Can you describe a bit more what "the number of input operations in the match section" is?
Is it the number of occurrences of op<> in the PDLL text or something else ?
It can't be too dynamic (e.g. # of Operation* matched or going through input operands and determining whether they come from BBAarg or Op) because the pattern is static.

351

This is not yet supported in code (in the PRs I saw saw far) IIRC ?

397

"to having a type" ?

430

Can this "first use" ever be different than first occurrence in program string ?
In any case, please specify it explicitly.

527

It is a bit unclear to me whether the point of definition is the PDLL statement or just the internal expression.

Can we write: let root = op<my_dialect.foo>(arg: Value, _: Value, _: Value, arg); ?

Also, side note, can we write: let root = op<my_dialect.foo>(arg: Value, _: [Value, Int64], _: [Value, Int32], arg); ?

563

Could we also support op_interface ?
Seems easily doable by using op<> + native constraint but:

  1. maybe there is a more efficient way to target that
  2. seems like it would be generally useful to avoid that boilerplate
602

In the future, do you envision this to help allow multiple Variadic packs more naturally than with all the special attribute genuflexions?

638

Same Q re packs and maybe also re. does this mix with type constraints: otherResults: [TypeRange, I32OrI64]

674

Do you see a future where tablegen is replace in MLIR (beyond what you are currently doing for TDRRs) ?

689

+1

715

Add an example with an AffineMapAttr to show its full power ?

785

Side Q, is there an easy way to get PDL-specific match and apply pattern failure messages without pulling in the whole -debug shabang?

835

Should there be some examples with the let value: [Value, I64] form ?

The variable constraint section says:

See the general [Constraints](#constraints) section for more detailed information.
nicolasvasilache accepted this revision.Feb 16 2022, 5:05 AM

Awesome, thanks much for this line of work!

mlir/docs/PDLL.md
835

Ah scratch this, this is touched on in "Defining Constraints Inline" below.

So given that:

PDLL Constraints and Native Constraints defined in
PDLL may be specified *inline* at any level of nesting

we should be able to write let root = op<my_dialect.foo>(arg: Value, _: [Value, Int64], _: [Value, Int32], arg); correct ?

1006

How would you do with passing PoD (e.g. an int argument to the constraint)?
Would we have to explicitly use an attribute and unpack it in C++ ?

Also, could you elaborate a bit on the debugging story?
Would we have to go look at the generated C++ in case of mismatches or do you envision good error messages at this level?

1119

This is quite implicit, what is the behavior on operands and attributes ?
Are they all forwarded from my_dialect.foo to my_dialect.bar ?

1157

We touched on this a while back during your presentation: for rewriters defined in PDLL, do you envision a primitive templating addition that would allow at least some unrolling and simple indexing arithmetic to occur?
This would be needed to move a bunch of non-trivial patterns to PDLL.

1185

Hmm the match part has a piece called Create....
Is this a misnomer or will it be able to produce IR

If the latter, does this all get properly reverted if some later condition fail to match?

Maybe document the expectations more explicitly in Language Specification > Patterns ?

Atm I see:

// Pattern bodies are separated into two components:
// * Match Section
//    - Describes the input IR.
//    - [Proposed] Is allowed / not allowed to create IR
... 
// * Rewrite Section
//    - Describes how to transform the IR.
//    - Last statement starts the rewrite.

This seems

1197

typo: inference

1255

Maybe sketch the C++ API this would hook to (e.g. is there a rewriter somewhere ?)

1289

Same Q as above re passing a PoD (e.g. int) around.

This revision is now accepted and ready to land.Feb 16 2022, 5:05 AM
rriddle updated this revision to Diff 409420.Feb 16 2022, 2:42 PM
rriddle updated this revision to Diff 414909.Mar 13 2022, 12:47 AM
rriddle marked 21 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2022, 12:47 AM
rriddle added inline comments.Mar 13 2022, 12:48 AM
mlir/docs/PDLL.md
325

Yeah, it's effectively the distinct number of Op variables/expressions in the input. Effectively the extent of statically guessing the size of the match.

351

Yep, still need to plumb this through PDL (will be in a followup commit).

430

Updated to make it more clear.

527

Yes to both. The point of definition is the point of reference expression added some more examples.

563

Yeah, we will likely end up supporting something like that if it becomes super common. The main hurdle before was the lack of enough information from ODS, which we have now. If this is something you think we'll want sooner rather than later, can you file an issue to track?

602

Do you mean in ODS, or in PDLL?

638

Is the second part of this about applying a single element constraint to all of the elements within the range?

674

It depends on how much usability win we get from it. Right now Tablegen seems to be "good enough" for what we want to do with most ODS things, given that ODS is much more focused on record definitions than pattern matching is. There is also a matter of whether we'd want to extend/"fix" tablegen vs make something new.

785

The only thing PDL specific is for the byte code, pdl-bytecode, otherwise all of the normal pattern debug logging should also apply.

835

Yep, I added some examples in the inline variable section about this.

1006

All constants passed to constraints will need to be passed through attributes. We can definitely develop some better APIs on the C++ side to, for example, unwrap IntegerAttr to something like int64_t/APInt/etc. The main thing is that because we go through PDL, we have to be able to encode things via attributes.

For debugging, there are various different factors involved. In the constraint, though this example doesn't show it (maybe it should?) you have access to the rewriter and could return a rewriter.notifyMatchFailure(...) in the case of failure. I think if the user doesn't, then we could likely generate some default messages based on the name of the constraint/how it was applied/etc. This is definitely something that we will want to have a dedicated effort into verifying/validating. It's also one of the things that we need a base end-to-end toolchain to start really experiencing. We'll have to ensure that we don't lose the debuggability aspect of patterns just because we go declarative. This is also something that we really need to figure out at the PDL level, given that PDL performs merging on the input sections (which drops a lot of the current match context, i.e. you can't really say "which pattern am I in?")

1157

Yeah, I can definitely foresee various different primitives that we'll want to develop in that area. The main thing is that we'll need use cases to figure out how exactly we want to structure it in the language, which contexts we support it, etc.

1185

This Create is actually within the rewrite block:

Pattern {
  rewrite ... with {
    // This is in a rewrite scope now.
  }
}
1255

Done. The intention here eventually is to link to the PDL documentation, which is the part that should describe this in further detail.

This revision was automatically updated to reflect the committed changes.