This revision updates the documentation for dialect conversion, as many concepts have changed/evolved over time.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
---|---|---|
130–134 | This sentence doesn't really say anything... Suggest: "If an operation is marked recursively legal, either statically or dynamically, then all of..." | |
193 | comma after space | |
218 | "and how to" Lacks parallel structure. | |
356–359 | 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 | ||
---|---|---|
189 | Nit: ; -> , ? | |
191 | -> replacements and erasures | |
193 | space after comma | |
316 | Is there a reason you use "casts" here instead of "converts"? | |
322 | The last clause "... may persist ... has finished" isn't clear. | |
325 | Is this stale? | |
351–354 | Should these paras be moved above the snippet above? | |
356–358 | Nice. |
Thanks for the reminder, added a section for that.
mlir/docs/DialectConversion.md | ||
---|---|---|
316 | Not particularly, they are used somewhat interchangeably. Switched to converts. | |
325 | That applies solely to the addArgumentMaterialization method, do you have a suggestion? | |
356–359 | 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 | ||
---|---|---|
277–281 | Is this what you added about failing type conversions? I can't quite parse it. I think you're saying: |
mlir/docs/DialectConversion.md | ||
---|---|---|
277–281 |
No, I added a section on Type Safety to Conversion Pattern.
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:
You don't know if a pattern will fail, and the materialization can happen after all patterns have already been applied. |
Thanks, River! This makes things more much clearer for the users.
mlir/docs/DialectConversion.md | ||
---|---|---|
129 | Nit: regions of operations -> regions | |
133 | 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. | |
225 | Nit: I'd drop the comma here | |
227 | Ultra-nit: let's keep backticks for the code. We can use _italics_ for highlights, e.g. _conversion_. | |
232 | An important difference is that a materialization can produce IR but a conversion cannot. | |
242 | 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. | |
269 | Typo: "converted provided" | |
277–281 | This seems to be missing a logical connection: "In the case of a target | |
432 | "needs to be legalized" sounds a bit weird here since it's declared legal by the target |
mlir/docs/DialectConversion.md | ||
---|---|---|
325 | I'll give it another read. Nothing now. |
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. |
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. |
Nit: regions of operations -> regions