Page MenuHomePhabricator

[mlir][DialectConversion] Update the documentation for dialect conversion
ClosedPublic

Authored by rriddle on Aug 3 2020, 3:24 PM.

Details

Summary

This revision updates the documentation for dialect conversion, as many concepts have changed/evolved over time.

Diff Detail

Event Timeline

rriddle created this revision.Aug 3 2020, 3:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2020, 3:24 PM
rriddle requested review of this revision.Aug 3 2020, 3:24 PM

One thing I think is missing from this when and if failed type conversions result in failed patterns. I was debugging a case recently where a type conversion failed, but it wasn't obvious why this happened, and it was not reported separately, only as part of a failing pattern.

mlir/docs/DialectConversion.md
128–134

This sentence doesn't really say anything... Suggest: "If an operation is marked recursively legal, either statically or dynamically, then all of..."

198

comma after space

223

"and how to" Lacks parallel structure.

367–370

It seems that Materializations are only part of partial conversions and not full conversions. Perhaps this should be explicit? Or at least for source materialization and target materialization?

This is quite useful - thanks. Some minor comments on the first half.

mlir/docs/DialectConversion.md
194

Nit: ; -> , ?

196

-> replacements and erasures

198

space after comma

327

Is there a reason you use "casts" here instead of "converts"?

333

The last clause "... may persist ... has finished" isn't clear.

336

Is this stale?

362–365

Should these paras be moved above the snippet above?

367–369

Nice.

rriddle updated this revision to Diff 282981.Aug 4 2020, 11:45 AM
rriddle marked 11 inline comments as done.

Resolve comments and a section on debugging

One thing I think is missing from this when and if failed type conversions result in failed patterns. I was debugging a case recently where a type conversion failed, but it wasn't obvious why this happened, and it was not reported separately, only as part of a failing pattern.

Thanks for the reminder, added a section for that.

mlir/docs/DialectConversion.md
327

Not particularly, they are used somewhat interchangeably. Switched to converts.

336

That applies solely to the addArgumentMaterialization method, do you have a suggestion?

367–370

A materialization can happen during any conversion type, didn't mean to to allude here that it was just during a partial conversion. Removed the use of partial, so that it says "during a conversion".

mlir/docs/DialectConversion.md
282–286

Is this what you added about failing type conversions? I can't quite parse it. I think you're saying:
If they (users of that definition) aren't (updated during the conversion process), then a type conversion must be materialized.... But that means that patterns fail? What's the point of materializing the conversion if patterns will still fail?

rriddle marked an inline comment as done.Aug 4 2020, 12:16 PM
rriddle added inline comments.
mlir/docs/DialectConversion.md
282–286

Is this what you added about failing type conversions? I can't quite parse it.

No, I added a section on Type Safety to Conversion Pattern.

If they (users of that definition) aren't (updated during the conversion process), then a type conversion must be materialized.... But that means that patterns fail?

No, because if the type of a definition has been changed the pattern has largely already been successfully applied. The only times that you can really tie failed materialization with a failed pattern are when it involves the remapped inputs. That is only one situation though.

There are two situations for when type materializations occur:

  • Materializing a conversion for the remapped input to a pattern. If the materialization here fails, the pattern will fail automatically and never reach the matchAndRewrite.
  • Materializing a conversion for an operation generated during a successful pattern match that is being used by an already legal operation. This is only possible to detect at the end of the conversion process, given that you don't know if all of the users of the original value are going to be updated when you initially applied the pattern.

What's the point of materializing the conversion if patterns will still fail?

You don't know if a pattern will fail, and the materialization can happen after all patterns have already been applied.

ftynse added a comment.Aug 5 2020, 3:14 AM

Thanks, River! This makes things more much clearer for the users.

mlir/docs/DialectConversion.md
128–129

Nit: regions of operations -> regions

131

Nit: use quotes rather than backticks for "illegal"

173

It may or may not update the operations in-place (pattern root operations can be updated using a mechanism AFAIR).

Disregard if it is mentioned below, but we should really mention that in-place op updates such as replaceAllUsesWith or setOperand are not allowed (except the special mechanism for root) in conversion patterns.

177

The type of an operation -> the types of values defined by an operation? There has been some confusion between operations being values and thus having types in the past.

179

types of the operands?

182

the set of values that were used to replace the original operands -> the list of operands that the operation should use after conversion?

Set/list is a semantic nit on the importance of the order, and the operands are not necessarily replaced.
It may also be worth mentioning that original operands are available and can be inspected.

230

Nit: I'd drop the comma here

232

Ultra-nit: let's keep backticks for the code. We can use _italics_ for highlights, e.g. _conversion_.

237

An important difference is that a materialization can produce IR but a conversion cannot.

247

I'd really appreciate an easy rule to remember what are source and target materializations. Something like: if a value is used by an op that isn't converted, it needs conversion at _source_ (or def) of the value, hence source materialization; if a value is used by an op that is being converted by has a wrong type, it needs conversion at _target_ (or use) of the value, hence target materialization.

274

Typo: "converted provided"

282–286

This seems to be missing a logical connection: "In the case of a target
materialization for the remapped inputs of a conversion pattern, the pattern application fails" sounds like "if target materialization happens, the pattern fails". It should be something like "If the target materialization is required, but cannot be performed, the pattern application fails".

443

"needs to be legalized" sounds a bit weird here since it's declared legal by the target

bondhugula added inline comments.Aug 5 2020, 10:17 AM
mlir/docs/DialectConversion.md
336

I'll give it another read. Nothing now.

rriddle updated this revision to Diff 283757.Aug 6 2020, 3:57 PM
rriddle marked 14 inline comments as done.

Address comments

rriddle added inline comments.Aug 6 2020, 9:37 PM
mlir/docs/DialectConversion.md
173

I have a followup revision that talks about the constraints on patterns in general. I'll add a link from here to the appropriate section in that revision.

232

I'd prefer to keep them for now, as this is consistent with the rest of the doc(and other docs).

ftynse accepted this revision.Aug 7 2020, 6:00 AM

Thanks!

This revision is now accepted and ready to land.Aug 7 2020, 6:00 AM
bondhugula accepted this revision.Aug 7 2020, 9:22 PM
bondhugula requested changes to this revision.Aug 11 2020, 3:18 AM
bondhugula added inline comments.
mlir/docs/DialectConversion.md
183–186

There is still one key thing that would not be clear here: what happens when there are no replacement operands available, i.e., the def's or the block arguments of the operands were not converted? Will those positions in operands be nullptr or would they be the current operand on the op? I believe it's the latter but the statement ... list of operands that the operation should use after conversion would make it ambiguous or imply nullptr, and this information I think can only be found by digging the implementation of dialect conversion.

This revision now requires changes to proceed.Aug 11 2020, 3:18 AM
rriddle updated this revision to Diff 285239.Aug 12 2020, 7:46 PM
rriddle marked an inline comment as done.

Resolve comments

mlir/docs/DialectConversion.md
183–186

Updated, if an operand belongs to an operation that wasn't converted the original operand is used, the entries are never null.

bondhugula accepted this revision.Aug 13 2020, 12:33 AM

Thanks for addressing everything.

This revision is now accepted and ready to land.Aug 13 2020, 12:33 AM