Page MenuHomePhabricator

[MLIR][SPIRV] Add initial support for OpSpecConstantOp.
ClosedPublic

Authored by ergawy on Nov 27 2020, 7:56 AM.

Details

Summary

This commit adds initial support for SPIR-V OpSpecConstantOp
instruction. The following is introdcued:

  • A new spv.specConstantOperation operation consisting of a single

region and of 2 operations within that regions (more details in the
docs of the op itself).

  • A new spv.yield instruction that acts a terminator for

spv.specConstantOperation.

For now, the generic form of the new op is supported (i.e. no custom
parsing or printing). This will be done in a follow up patch.

Diff Detail

Event Timeline

ergawy created this revision.Nov 27 2020, 7:56 AM
ergawy requested review of this revision.Nov 27 2020, 7:56 AM

This is an initial patch to make sure I didn't anything disastrously wrong :D.

ergawy updated this revision to Diff 308365.Nov 30 2020, 7:26 AM
This comment was removed by ergawy.
ergawy updated this revision to Diff 308395.Nov 30 2020, 9:20 AM

Rename to spv.mlir.yield.

antiagainst requested changes to this revision.Dec 1 2020, 12:05 PM

Nice! Just a few nits.

mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
612

Yields the result computed in .. region back to .. op

643

Could you use the symbol SpecConstantOperation? (So capitalize the first letter.) (We have a bunch other ops starting with small cases but we should change them to all capitalized to be consistent.)

643

Wrap around 80 characters

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
3412

We've checked this in the enclosing op. We can drop the check here?

This revision now requires changes to proceed.Dec 1 2020, 12:05 PM
antiagainst added inline comments.Dec 1 2020, 2:41 PM
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
3443

This list is fine for now; but it would be nice that if we can have a following up CL to introduce an op trait, like spirv::UsableInSpecConstantOp and define its counterparts in TableGen so that we can mark those op themselves.

You can find examples of op traits in https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpDefinition.h#L684 and the corresonding TableGen definition in https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpBase.td#L1749.

ergawy updated this revision to Diff 308937.Dec 2 2020, 4:59 AM
ergawy marked 4 inline comments as done.

Handle review comments.

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
3412

The verifier for SpecCosntantOperation below doesn't have that check actually. I think doing this here will provide a more localized error message.

3443

Added a TODO.

ergawy updated this revision to Diff 309821.Dec 7 2020, 12:34 AM
  • Add IsolatedFromAbove trait to spv.SpecConstantOperation.
ergawy added inline comments.Dec 7 2020, 12:37 AM
mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
646

Please feel free to disagree with adding this trait. I added this to make sure we have tight control on what values get used inside the SpecConstantOperation's region and avoid capturing values from outside that aren't not constants.

Verification logic is simpler and more robust this way, I guess.

antiagainst accepted this revision.Dec 8 2020, 6:00 AM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVStructureOps.td
646

Looks good!

This revision is now accepted and ready to land.Dec 8 2020, 6:00 AM
This revision was landed with ongoing or failed builds.Dec 8 2020, 6:09 AM
This revision was automatically updated to reflect the committed changes.