This is an archive of the discontinued LLVM Phabricator instance.

[mlir:PDLL] Add proper support for operation result type inference
ClosedPublic

Authored by rriddle on May 2 2022, 10:16 AM.

Details

Summary

This allows for the results of operations to be inferred in certain contexts,
and matches the support in PDL for result type inference. The main two
initial circumstances are when used as a replacement of another operation,
or when the operation being created implements InferTypeOpInterface.

Diff Detail

Event Timeline

rriddle created this revision.May 2 2022, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2022, 10:16 AM
rriddle requested review of this revision.May 2 2022, 10:16 AM
rriddle updated this revision to Diff 426837.May 3 2022, 2:30 PM
rriddle updated this revision to Diff 429570.May 15 2022, 3:24 PM

(first sweep, mostly looks good, just some clarifications)

mlir/docs/PDLL.md
729

Are the other contexts not bound types? E.g, isn't this the only inferred case and the others bound/provided?

mlir/lib/Tools/PDLL/Parser/Parser.cpp
83

What about Explicit, Replacement, Inferred ?

2063

Do we also need to check that the op implements the interface?

2779

For a replacement don't we need this too? Or perhaps stepping up: what verification is done here and why doesn't it apply for replacement?

2786

I'd make this fail: given this is an AOT tool, we could warn or fail here always rather than asserting, especially as this doesn't sound like an invariant.

2822

Runtime runtime? E.g., are you thinking about dynamically registered/injected interfaces?

2839

Why only check for variadic?

rriddle updated this revision to Diff 430287.May 18 2022, 2:19 AM
rriddle marked 7 inline comments as done.
rriddle added inline comments.May 18 2022, 2:20 AM
mlir/docs/PDLL.md
729

Technically I suppose, it depends on the context of "inference". From PDLL perspective, it is inferring the types from somewhere (even if from the MLIR level, it's using bound/provided types). The use of "inference" in PDLL is when the user doesn't explicitly provide result types.

mlir/lib/Tools/PDLL/Parser/Parser.cpp
2063

We handle the related verification in createOperationExpr when creating the expression, the parser here tries not to inject any of that information (given that when templates are in the mix, we might not know what the operation is here).

2779

The replacement inference case only happens when there are no explicit results (we don't infer if results were explicitly provided), so we don't have anything to verify in that case. The verification here is that the structure of the result expressions matches the expected format (either single value range or matching the ODS definition).

2786

It's an assert right now because there isn't a code path that would exercise the error. We don't set inference if explicit values were provided. I avoided an error because that gives the expectation that the path is expected/tested/thought out, as opposed to something that can't happen. Reworked the assert to better capture this.

2822

Yes, I'm thinking of dynamically registered interfaces. We do have some instances of injecting type inference interfaces, though we could choose, I suppose, to require that the ODS definition has them. Right now we haven't been applying hard enforcement that ODS has everything defined.

2839

Purely variable length result operations can have zero results, and we allow elided results lists to imply zero results.

rriddle updated this revision to Diff 430847.May 19 2022, 4:38 PM
rriddle updated this revision to Diff 431751.May 24 2022, 12:22 PM
jpienaar accepted this revision.May 28 2022, 11:23 AM

Looks good, thanks!

This revision is now accepted and ready to land.May 28 2022, 11:23 AM