[PATCH] D76244: [mlir][spirv] Make SPIRVTypeConverter target environment aware

[Mahesh Ravishankar via Phabricator via llvm-commits]

mravishankar requested changes to this revision.
mravishankar added a comment.
This revision now requires changes to proceed.

Awesome! THis is a very clean solution



================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h:38
+///
+/// TODO(antiagainst): We might want to introduce a way to control how
+/// unsupported bitwidth are handled and explicitly fail if wanted.
----------------
Cant we do this right now? If any tests failed we can update the extension/capability accordingly and not just force expand right?


================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp:285
+
+static Optional<Type> convertTensorType(const spirv::TargetEnv &targetEnv,
+                                        TensorType type) {
----------------
I understand that this is maintaining functionality. I am actually not sure it is a good thing to lower from TensorType directly into SPIR-V type. SPIRV Array Type is not an SSA data-structure. It is a load/store data-structure. Please remove this. We should never have supported this in the first place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76244/new/

https://reviews.llvm.org/D76244