This is an archive of the discontinued LLVM Phabricator instance.

[mlir:PDLInterp] Refactor the implementation of result type inferrence
ClosedPublic

Authored by rriddle on Apr 27 2022, 5:35 PM.

Details

Summary

The current implementation uses a discrete "pdl_interp.inferred_types"
operation, which acts as a "fake" handle to a type range. This op is
used as a signal to pdl_interp.create_operation that types should be
inferred. This is terribly awkward and clunky though:

  • This op doesn't have a byte code representation, and its conversion to bytecode kind of assumes that it is only used in a certain way. The current lowering is also broken and seemingly untested.
  • Given that this is a different operation, it gives off the assumption that it can be used multiple times, or that after the first use the value contains the inferred types. This isn't the case though, the resultant type range can never actually be used as a type range.

This commit refactors the representation by removing the discrete
InferredTypesOp, and instead adds a UnitAttr to
pdl_interp.CreateOperation that signals when the created operations
should infer their types. This leads to a much much cleaner abstraction,
a more optimal bytecode lowering, and also allows for better error
handling and diagnostics when a created operation doesn't actually
support type inferrence.

Depends on D124586

Diff Detail

Event Timeline

rriddle created this revision.Apr 27 2022, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2022, 5:35 PM
rriddle requested review of this revision.Apr 27 2022, 5:35 PM
rriddle edited the summary of this revision. (Show Details)
jpienaar accepted this revision.Apr 29 2022, 7:46 AM

Makes sense to have some marker here, its a hole in the type description SGTM

mlir/lib/Conversion/PDLToPDLInterp/PDLToPDLInterp.cpp
107

Could we interleaving mutated/return values with read only values?

mlir/lib/Dialect/PDLInterp/IR/PDLInterp.cpp
51

Early return?

mlir/lib/Rewrite/ByteCode.cpp
850

Could you create constant for sentinel that is easier to identify C++ side?

1531

That would make this less magical

This revision is now accepted and ready to land.Apr 29 2022, 7:46 AM
This revision was landed with ongoing or failed builds.May 1 2022, 12:47 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked 4 inline comments as done.