This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add VectorExtractDynamicOp and vector.extractelement lowering
ClosedPublic

Authored by abialas on Nov 3 2020, 5:15 AM.

Details

Summary
  • VectorExtractDynamicOp in SPIRV dialect
  • conversion from vector.extractelement to spirv VectorExtractDynamicOp

Diff Detail

Event Timeline

abialas created this revision.Nov 3 2020, 5:15 AM
abialas requested review of this revision.Nov 3 2020, 5:15 AM
ThomasRaoux added inline comments.Nov 3 2020, 9:14 AM
mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
89

I think we need to check that the original vector type is converted to a spv.vector. Large vector would be converted to an array. You can use !spirv::CompositeType::isValid(insertOp.getDestVectorType() as i the pattern above. You may also want a negative test for this case.

antiagainst requested changes to this revision.Nov 3 2020, 10:50 AM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVOps.td
532 ↗(On Diff #302549)

Can this be put in SPIRVCompositeOps.td? We are basically following the spec's group regarding file organization. (This file is a bit different but it should be cleaned up..)

574 ↗(On Diff #302549)

This cannot be aritary SPIR-V types? It should be SPV_Scalar?

mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
89

+1. BTW, if you want to test out large vectors that are enabled by the Vector16 extension, you'll need to have proper spv.target_env that enables Vector16 extension. Otherwise it won't convert. You can find examples of how to specify such target env in various -> spirv conversion mlir files.

This revision now requires changes to proceed.Nov 3 2020, 10:50 AM
abialas updated this revision to Diff 302791.Nov 4 2020, 2:27 AM

Thank You for the comments! Added requested changes. I wanted to keep this change not dependent on the Vector16 change, so did not add the unit test for VectorExtractDynamicOp with vector sizes 8 or 16, but verified that it is working in our use cases.

antiagainst accepted this revision.Nov 4 2020, 4:42 AM

LGTM. Just one final nit: the tests for op and serialization should be added into composite-ops.mlir now given the op is defined in SPIRVCompositeOps.td.

This revision is now accepted and ready to land.Nov 4 2020, 4:42 AM
abialas updated this revision to Diff 302817.Nov 4 2020, 4:52 AM

moved tests to composite-ops

ThomasRaoux accepted this revision.Nov 4 2020, 8:33 AM