[PATCH] D95580: [mlir][spirv] Add support for OpImageType

[Lei Zhang via Phabricator via llvm-commits]

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

Looks awesome! I just have a few nits here. Thanks for pushing on it!



================
Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp:1013
+  if (operands.size() != 8)
+    return emitError(unknownLoc, "OpTypeImage must have eight operands");
+
----------------
Nit: It's not that OpTypeImage must have eight operands (as per the spec). I think it's more that "OpTypeImage with non-eight operands are not supported yet". :)


================
Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp:1021
+  auto dim = spirv::symbolizeDim(operands[2]);
+  auto depthInfo = spirv::symbolizeImageDepthInfo(operands[3]).getValue();
+  auto arrayedInfo = spirv::symbolizeImageArrayedInfo(operands[4]).getValue();
----------------
We should check the `Optional` and return errors. Directly `getValue` can crash given bad SPIR-V input.


================
Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp:1027
+
+  if (dim == spirv::Dim::SubpassData) {
+    if (samplerUseInfo != spirv::ImageSamplerUseInfo::NoSampler ||
----------------
This should be checked in the type itself (like using `verifyConstructionInvariants`). In general the deserializer is a tool that reads in SPIR-V blob and generates SPIR-V IR. For the deserializer's concern it should only care about the input binary's validity: whether the input blob is encoded properly. Checks for semantic rules should be on the type/op itself. This separates concerns. Also generally in IR form we can have better error report (like getting the exact IR dumped with line #), where we cannot get it here. :)


================
Comment at: mlir/test/Target/SPIRV/image.mlir:3
+
+spv.module Logical GLSL450 requires #spv.vce<v1.0, [Shader], []> {
+  // CHECK: !spv.ptr<!spv.image<f32, Dim1D, NoDepth, NonArrayed, SingleSampled, NoSampler, Unknown>, UniformConstant>
----------------
I feel we don't need all these combinations: if one enumerant works, others in the same enum will work too. So a few cases to vary as much as possible would be good enough. :)


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

https://reviews.llvm.org/D95580