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

[Weiwei via Phabricator via llvm-commits]

Weiwei-2021 marked 4 inline comments as done.
Weiwei-2021 added inline comments.


================
Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp:1013
+  if (operands.size() != 8)
+    return emitError(unknownLoc, "OpTypeImage must have eight operands");
+
----------------
antiagainst wrote:
> 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". :)
Good point. Thanks!


================
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();
----------------
antiagainst wrote:
> We should check the `Optional` and return errors. Directly `getValue` can crash given bad SPIR-V input.
Thanks for pointing this out! Curently,  I checked each operand separately. Hope this is fine. 


================
Comment at: mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp:1027
+
+  if (dim == spirv::Dim::SubpassData) {
+    if (samplerUseInfo != spirv::ImageSamplerUseInfo::NoSampler ||
----------------
antiagainst wrote:
> 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. :)
Thanks for the explanation. I decide to delete this part from this function. Maybe we need to consider this in some other stage, but like you said, that is a separated concern. :)


================
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>
----------------
antiagainst wrote:
> 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. :)
Only keep three combinations here. Notice that the var number is unordered. I will fixed it later. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95580