This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add Vector to SPIR-V conversion pass
ClosedPublic

Authored by ThomasRaoux on Oct 2 2020, 4:06 PM.

Details

Summary

Add conversion pass for Vector dialect to SPIR-V dialect and add some simple conversion pattern for vector.broadcast, vector.insert, vector.extract.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Oct 2 2020, 4:06 PM
ThomasRaoux requested review of this revision.Oct 2 2020, 4:06 PM
antiagainst requested changes to this revision.Oct 5 2020, 1:49 PM

Nice! Thanks Thomas for starting this! I wanted to do this for ages. :)

mlir/include/mlir/Conversion/Passes.td
389

s/SPIRV/SPIR-V/

mlir/include/mlir/Conversion/VectorToSPIRV/ConvertVectorToSPIRVPass.h
10

SPIR-V

mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

Should we check that the input is just a scalar and not some n-D vector? Vector in SPIR-V is constrained; we can generate invalid SPIR-V ops here.

Similarly for the following two patterns.

86

Use empty lines to separate the function and namespace. (Won't clang-format do that actually?)

mlir/test/Conversion/VectorToSPIRV/simple.mlir
4

CHECK-LABEL

16

CHECK-LABEL

This revision now requires changes to proceed.Oct 5 2020, 1:49 PM

Address review comments.

ThomasRaoux marked 4 inline comments as done.Oct 5 2020, 3:11 PM

Thanks Lei. Please take another look.

mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

Good point. I added a check. There was already a check for the other patterns so I don't think I need anything else.

86

Yeah somehow clang-format doesn't seem to add it.

mravishankar accepted this revision.Oct 5 2020, 9:55 PM
antiagainst added inline comments.Oct 6 2020, 6:47 AM
mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

Vectors in SPIR-V can only be <= 4 elements by default. If the source is broadcasting a scalar into a, say, 128-element vector, it will create invalid SPIR-V. I think we also need to check the result vector is okay with spirv::CompositeType::isValid and otherwise return false. We can handle the large vector case later with type conversions.

Address review comment

ThomasRaoux added inline comments.Oct 6 2020, 8:57 AM
mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

I added check that the vector result is of size <= 4. Can the composite type not be valid for this case?
(I did the same for the other patterns)

antiagainst accepted this revision.Oct 6 2020, 10:28 AM
antiagainst added inline comments.
mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

Can the composite type not be valid for this case?

spirv::CompositeType::isValid(VectorType) is a static function that wraps around the logic here and checks whether a general VectorType is allowed in SPIR-V: https://github.com/llvm/llvm-project/blob/master/mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp#L174-L177. It can avoid us duplicating the logic multiple times. :)

This revision is now accepted and ready to land.Oct 6 2020, 10:28 AM

Use spirv::CompositeType::isValid

mlir/lib/Conversion/VectorToSPIRV/VectorToSPIRV.cpp
38

My bad I should have looked at it. Makes sense, I replaced the checks with this function.

This revision was automatically updated to reflect the committed changes.