This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add parsing and printing support for SpecConstantOperation
ClosedPublic

Authored by ergawy on Dec 9 2020, 12:59 AM.

Details

Summary

Adds more support for SpecConstantOperation by defining a custom
syntax for the op and implementing its parsing and printing.

Diff Detail

Event Timeline

ergawy created this revision.Dec 9 2020, 12:59 AM
ergawy requested review of this revision.Dec 9 2020, 12:59 AM

This custom assembly format is okay but I think we can further simplify it given we have very rigid structure inside the region. What about something like

%result = spv.SpecConstantOperation wraps "spv.IAdd"(%lhs, %rhs) : (i32, i32) -> i32

?

The spv.mlir.yield can then be implicitly constructed during parsing and omitted during printing.

We have examples for this in TensorFlow: tf_executor.island. See its parsing and printing function here:

And examples here: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/mlir/tensorflow/tests/executor_island_coarsening.mlir#L183-L188

Let me know how do you think. :)

mravishankar accepted this revision.Dec 14 2020, 1:28 PM

Thanks for the patch and sorry for the delay. Thanks @antiagainst for reviewing it. Just a minor comment above, but looks good to me.

mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
648–649

Why not add this tag SingleBlockImplicitTerminator<"SPIRV::YieldOp">

This revision is now accepted and ready to land.Dec 14 2020, 1:28 PM
ergawy updated this revision to Diff 311825.Dec 15 2020, 12:53 AM
  • Add more traits to aid verification.
  • Change custom syntax.
ergawy marked an inline comment as done.Dec 15 2020, 12:56 AM

This custom assembly format is okay but I think we can further simplify it given we have very rigid structure inside the region. What about something like

%result = spv.SpecConstantOperation wraps "spv.IAdd"(%lhs, %rhs) : (i32, i32) -> i32

?

The spv.mlir.yield can then be implicitly constructed during parsing and omitted during printing.

We have examples for this in TensorFlow: tf_executor.island. See its parsing and printing function here:

And examples here: https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/mlir/tensorflow/tests/executor_island_coarsening.mlir#L183-L188

Let me know how do you think. :)

That makes the syntax more concise and almost reduces it to its essence. Thanks for links. Followed a similar approach to TF.

mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
648–649

Thanks for bringing my attention to this, didn't know it existed. Add it and HasParent<SpecConstantOperationOp> as well.

ergawy updated this revision to Diff 311828.Dec 15 2020, 1:11 AM
ergawy marked an inline comment as done.

Revert unneeded changes.

ergawy updated this revision to Diff 311834.Dec 15 2020, 1:21 AM

Rebase and erase extra space.

ergawy updated this revision to Diff 312162.Dec 16 2020, 2:25 AM

Rebase to see if CI bug is resolved.

ergawy edited the summary of this revision. (Show Details)Dec 16 2020, 2:26 AM
antiagainst accepted this revision.Dec 16 2020, 5:19 AM
This revision was landed with ongoing or failed builds.Dec 16 2020, 5:27 AM
This revision was automatically updated to reflect the committed changes.