Page MenuHomePhabricator

[mlir][spirv] Support querying type extension/capability requirements
ClosedPublic

Authored by antiagainst on Mar 9 2020, 2:09 PM.

Details

Summary

Previously we only consider the version/capability/extension requirements
on ops themselves. Some types in SPIR-V also require special extensions
or capabilities to be used. For example, non-32-bit integers/floats
will require different capabilities and/or extensions depending on
where they are used because it may mean special hardware abilities.

This commit adds query methods to SPIR-V type class hierarchy to support
querying extensions and capabilities. We don't go through ODS for
auto-generating such information given that we don't have them in
SPIR-V machine readable grammar and there are just a few types.

Depends On D75874

Diff Detail

Event Timeline

antiagainst created this revision.Mar 9 2020, 2:09 PM
Herald added a project: Restricted Project. · View Herald Transcript

This is really great. I see there is a lot of complexity involved in deciding the extensions and capability, and storage class, etc. Wondering if most of the logic could be put in a single place for all types. So instead of querying the type to get the extension/capability info, have a single method that has all the logic...

Just making sure that this is still being worked on, and there are updates coming (the earlier comment doesnt seem to addressed, so wondering if I missed something)

mravishankar requested changes to this revision.Mar 11 2020, 12:07 PM
This revision now requires changes to proceed.Mar 11 2020, 12:07 PM

This is really great. I see there is a lot of complexity involved in deciding the extensions and capability, and storage class, etc. Wondering if most of the logic could be put in a single place for all types. So instead of querying the type to get the extension/capability info, have a single method that has all the logic...

You can have availability information on many things in SPIR-V, including types, ops, and enum cases, and these information are available from SPIR-V's human-readable grammar. So it's quite suitable for auto-generation. The way it works right now is we generate utility functions for each enum to return its requirements on version/extension/capabilities and also auto-generate the methods for ops to synthesize the availability info (note that for an op, the opcode itself has availability constraints, but it should also consider the enum attributes it has. Types are a bit different because we don't have the information readily available in the grammar. We can probably include that in the grammar too but it seems fine for now to manually write the C++ code given there are just a few types.

Hope the above clarifies things. I plan to write it down in the doc afterwards. Putting in a single place means we will have a gigantic method and it involves recursively calling into itself and goes through switch table all the time, which I don't think is a good practice.

Just making sure that this is still being worked on, and there are updates coming (the earlier comment doesnt seem to addressed, so wondering if I missed something)

Yep; still working on this chain of CLs. For now this one should be good for the batch; you can hold for reviews on next CL, which will need more changes to make sense. Sorry for the delay in addressing the comments. :)

mravishankar accepted this revision.Mar 12 2020, 2:43 PM

Just a few nits. Nice to see everything threaded through!

mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
276

THis is a nice addition!

mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp
583

So input/output cant be 8 bit?

mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
76

This and the above method are the same. Could just template this to reduce bloat

This revision is now accepted and ready to land.Mar 12 2020, 2:43 PM
rriddle added inline comments.Mar 12 2020, 3:19 PM
mlir/include/mlir/Dialect/SPIRV/SPIRVTypes.h
291

typo: cconstructors

mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
59

nit: Return the error directly.

61

nit: I don't think the join is actually necessary.

93

Same here.

antiagainst marked 10 inline comments as done.Mar 12 2020, 4:40 PM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVTypes.cpp
583

No. SPV_KHR_8bit_storage does not support 8-bit on Input/Output. Input/Output is not useful for compute pipeline where you can have only one compute shader. It's more for graphics pipeline where you've many shader stages and they pass data from one to another with Input/Output.

mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp
61

Hmm, I'm seeing compilation errors if missing it.

76

I've tried too. But we don't have a templated version of stringifyEnum<EnumClass> like what we have for symbolizeEnum<EnumClass>. I guess it's not really worth it for just two enum classes.

This revision was automatically updated to reflect the committed changes.
antiagainst marked 3 inline comments as done.