Page MenuHomePhabricator

[mlir][ODS] Support result type inference in custom assembly format
ClosedPublic

Authored by trilorez on Oct 7 2021, 10:32 AM.

Details

Summary

Operations that have the InferTypeOpInterface trait can now omit the return
types in their custom assembly formats.

Diff Detail

Event Timeline

trilorez created this revision.Oct 7 2021, 10:32 AM
trilorez requested review of this revision.Oct 7 2021, 10:32 AM
trilorez updated this revision to Diff 377912.EditedOct 7 2021, 10:50 AM

Ran clang-format, thought I had ran it before

Mogball added inline comments.
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1560

Can you move hoist this to the top as a const char *const and add a small snippet of documentation like the other code snippets?

trilorez updated this revision to Diff 377925.Oct 7 2021, 11:36 AM

Moved infer return type parser code snippet to a constant

rriddle added inline comments.Oct 7 2021, 12:35 PM
mlir/test/lib/Dialect/Test/TestOps.td
2028–2041

Can you simplify this to just infer a constant type? That would remove the need to add inputs.

mlir/tools/mlir-tblgen/OpFormatGen.cpp
693
1560

nit: Drop the trivial braces here.

2728
2737

What about situations where only some types can be resolved this way? Should we just default to using inferResultTypes if the user doesn't specify any results in the format?

lattner added a subscriber: lattner.Oct 7 2021, 2:17 PM

This looks great @trilorez ! Thank you for working on this

trilorez updated this revision to Diff 378032.Oct 7 2021, 3:41 PM

Simplified FormatInferTypeOp to remove inputs, which led to a fix for handling
zero operand operations.

Addressed other pr comments.

trilorez added inline comments.Oct 7 2021, 3:46 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
2737

The resolvers for result types can only be set by AllTypesMatch, SameOperandsAndResultType, and TypesMatchWith, which look like they can't have partial type inference.

For other cases, like one of several result types being manually specified, I'm not sure how partial type inference would work. I suppose already inferred types could be passed to inferReturnTypes along with the operands, but then they'd need to either be in order or pass along their index.

trilorez updated this revision to Diff 378698.Oct 11 2021, 9:26 AM

Added InferTypeOpInterface section to OpDefintions.md

rriddle added inline comments.Oct 11 2021, 10:51 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
2737

I was more saying that if the user didn't explicitly add a type to the format, we could just always use inferResultTypes(for all of the types, ignoring the other resolvers). I think it would be problematic if the user had one result that was a constant(e.g. Index or something), but had others that required being referred. Right now we would never give them a chance to infer the other result types.

trilorez updated this revision to Diff 378749.Oct 11 2021, 11:50 AM

Prefer inferring result types over using buildable type builders

trilorez updated this revision to Diff 378752.Oct 11 2021, 11:59 AM

Infer all result types if none are specified

trilorez added inline comments.Oct 11 2021, 12:00 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
2737

Ah, I think I understand now. I believe the latest update addresses this.

rriddle accepted this revision.Oct 11 2021, 12:43 PM

LG, thanks.

This revision is now accepted and ready to land.Oct 11 2021, 12:43 PM