[mlir][spirv] Add support for OpImageType

[Weiwei-2021]

Summary

Support OpImageType in SPIRV Dialect.

This change doesn't support operand AccessQualifier since it is optinal and only enables under Kernel capability.

co-authored-by: Alan Liu <[e-mail]>

[ergawy]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1012

small nit: I think the following if condition is more appropriate for this check. We can remove this line.

(Hope you don't mind me passing by :) ).

[Weiwei-2021]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1012

:) Thank you for reviewing! I will remove this assertion and upload again (after I figure out how to upload a revision. This is my first time using this system - still learning. :p)

[ergawy]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1012

One way to do that is:

• Do whatever changes on your working copy (i.e. remove the assert in this case).
• git add <file> and git commit --amend.
• arc diff origin/main, arcanist should detect that you want to update this revision (rather than open a new one).

So, basically, updating a revision is the same flow as opening a new one with the exception of amending to the already existing commit (most of the time at least). Hope that helps.

[Weiwei-2021]

Remove a redundant assertion.

[Weiwei-2021]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1012

Thank you for the explanation! I updated it.

[antiagainst]

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

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1013

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". :)

1021

We should check the Optional and return errors. Directly getValue can crash given bad SPIR-V input.

1027

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. :)

mlir/test/Target/SPIRV/image.mlir

4

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. :)

[Weiwei-2021]

Apply some changes based on comments.

[Weiwei-2021]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1013

Good point. Thanks!

1021

Thanks for pointing this out! Curently, I checked each operand separately. Hope this is fine.

1027

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. :)

mlir/test/Target/SPIRV/image.mlir

4

Only keep three combinations here. Notice that the var number is unordered. I will fixed it later.

[Weiwei-2021]

Fix a numbering issue in test.

mlir/test/Target/SPIRV/image.mlir

4

Only keep three combinations here. Notice that the var number is unordered. I will fixed it later.

fix the numbering now.

[antiagainst]

Awesome, just one final nit from me.

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1023

It isn't missing. It's "unknown Dim for OpTypeImage: "

Similarly for others. :)

[Weiwei-2021]

Correct one work for error handling

[Weiwei-2021]

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

1023

Good catch! Thank you.

[antiagainst]

Awesome, thanks!

[antiagainst]

Oops, sorry the patch (https://github.com/llvm/llvm-project/commit/21f1462106b9ee1e646bf409c85528828320b34e) gotten landed has incorrect author. So I reverted it. Could you let me know the Author field I should use for submitting the patch?

[Weiwei-2021]

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <[e-mail]>".

[antiagainst]

In D95580#2534580, @Weiwei-2021 wrote:

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <[e-mail]>".

Awesome, thanks! Fixed and relanded as https://github.com/llvm/llvm-project/commit/35f746c17fd0315b235050c0a350fc521f944f20.

[Weiwei-2021]

In D95580#2534844, @antiagainst wrote:

In D95580#2534580, @Weiwei-2021 wrote:

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <[e-mail]>".

Awesome, thanks! Fixed and relanded as https://github.com/llvm/llvm-project/commit/35f746c17fd0315b235050c0a350fc521f944f20.

Thank you for all your help on it! I will start contributing more our work on MLIR.

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVBase.td

 def SPV_OC_OpExecutionMode             : I32EnumAttrCase<"OpExecutionMode", 16>;
 def SPV_OC_OpCapability                : I32EnumAttrCase<"OpCapability", 17>;
 def SPV_OC_OpTypeVoid                  : I32EnumAttrCase<"OpTypeVoid", 19>;
 def SPV_OC_OpTypeBool                  : I32EnumAttrCase<"OpTypeBool", 20>;
 def SPV_OC_OpTypeInt                   : I32EnumAttrCase<"OpTypeInt", 21>;
 def SPV_OC_OpTypeFloat                 : I32EnumAttrCase<"OpTypeFloat", 22>;
 def SPV_OC_OpTypeVector                : I32EnumAttrCase<"OpTypeVector", 23>;
 def SPV_OC_OpTypeMatrix                : I32EnumAttrCase<"OpTypeMatrix", 24>;
+def SPV_OC_OpTypeImage                 : I32EnumAttrCase<"OpTypeImage", 25>;
 def SPV_OC_OpTypeArray                 : I32EnumAttrCase<"OpTypeArray", 28>;
 def SPV_OC_OpTypeRuntimeArray          : I32EnumAttrCase<"OpTypeRuntimeArray", 29>;
 def SPV_OC_OpTypeStruct                : I32EnumAttrCase<"OpTypeStruct", 30>;
 def SPV_OC_OpTypePointer               : I32EnumAttrCase<"OpTypePointer", 32>;
 def SPV_OC_OpTypeFunction              : I32EnumAttrCase<"OpTypeFunction", 33>;
 def SPV_OC_OpTypeForwardPointer        : I32EnumAttrCase<"OpTypeForwardPointer", 39>;
 def SPV_OC_OpConstantTrue              : I32EnumAttrCase<"OpConstantTrue", 41>;
 def SPV_OC_OpConstantFalse             : I32EnumAttrCase<"OpConstantFalse", 42>;

 
 def SPV_OpcodeAttr :
     SPV_I32EnumAttr<"Opcode", "valid SPIR-V instructions", [
       SPV_OC_OpNop, SPV_OC_OpUndef, SPV_OC_OpSourceContinued, SPV_OC_OpSource,
       SPV_OC_OpSourceExtension, SPV_OC_OpName, SPV_OC_OpMemberName, SPV_OC_OpString,
       SPV_OC_OpLine, SPV_OC_OpExtension, SPV_OC_OpExtInstImport, SPV_OC_OpExtInst,
       SPV_OC_OpMemoryModel, SPV_OC_OpEntryPoint, SPV_OC_OpExecutionMode,
       SPV_OC_OpCapability, SPV_OC_OpTypeVoid, SPV_OC_OpTypeBool, SPV_OC_OpTypeInt,
-      SPV_OC_OpTypeFloat, SPV_OC_OpTypeVector, SPV_OC_OpTypeMatrix,
+      SPV_OC_OpTypeFloat, SPV_OC_OpTypeVector, SPV_OC_OpTypeMatrix, SPV_OC_OpTypeImage,
       SPV_OC_OpTypeArray, SPV_OC_OpTypeRuntimeArray, SPV_OC_OpTypeStruct,
       SPV_OC_OpTypePointer, SPV_OC_OpTypeFunction, SPV_OC_OpTypeForwardPointer,
       SPV_OC_OpConstantTrue, SPV_OC_OpConstantFalse, SPV_OC_OpConstant,
       SPV_OC_OpConstantComposite, SPV_OC_OpConstantNull, SPV_OC_OpSpecConstantTrue,
       SPV_OC_OpSpecConstantFalse, SPV_OC_OpSpecConstant,
       SPV_OC_OpSpecConstantComposite, SPV_OC_OpSpecConstantOp, SPV_OC_OpFunction,
       SPV_OC_OpFunctionParameter, SPV_OC_OpFunctionEnd, SPV_OC_OpFunctionCall,
       SPV_OC_OpVariable, SPV_OC_OpLoad, SPV_OC_OpStore, SPV_OC_OpCopyMemory,

mlir/lib/Target/SPIRV/Deserialization/DeserializeOps.cpp

LogicalResult spirv::Deserializer::processInstruction(
   case spirv::Opcode::OpTypeVoid:
   case spirv::Opcode::OpTypeBool:
   case spirv::Opcode::OpTypeInt:
   case spirv::Opcode::OpTypeFloat:
   case spirv::Opcode::OpTypeVector:
   case spirv::Opcode::OpTypeMatrix:
   case spirv::Opcode::OpTypeArray:
   case spirv::Opcode::OpTypeFunction:
+  case spirv::Opcode::OpTypeImage:
   case spirv::Opcode::OpTypeRuntimeArray:
   case spirv::Opcode::OpTypeStruct:
   case spirv::Opcode::OpTypePointer:
   case spirv::Opcode::OpTypeCooperativeMatrixNV:
     return processType(opcode, operands);
   case spirv::Opcode::OpTypeForwardPointer:
     return processTypeForwardPointer(operands);
   case spirv::Opcode::OpConstant:

mlir/lib/Target/SPIRV/Deserialization/Deserializer.h

private:
   LogicalResult processOpTypePointer(ArrayRef<uint32_t> operands);
 
   LogicalResult processArrayType(ArrayRef<uint32_t> operands);
 
   LogicalResult processCooperativeMatrixType(ArrayRef<uint32_t> operands);
 
   LogicalResult processFunctionType(ArrayRef<uint32_t> operands);
 
+  LogicalResult processImageType(ArrayRef<uint32_t> operands);
+
   LogicalResult processRuntimeArrayType(ArrayRef<uint32_t> operands);
 
   LogicalResult processStructType(ArrayRef<uint32_t> operands);
 
   LogicalResult processMatrixType(ArrayRef<uint32_t> operands);
 
   LogicalResult processTypeForwardPointer(ArrayRef<uint32_t> operands);
 

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

case spirv::Opcode::OpTypePointer: {
     return processOpTypePointer(operands);
   } break;
   case spirv::Opcode::OpTypeArray:
     return processArrayType(operands);
   case spirv::Opcode::OpTypeCooperativeMatrixNV:
     return processCooperativeMatrixType(operands);
   case spirv::Opcode::OpTypeFunction:
     return processFunctionType(operands);
+  case spirv::Opcode::OpTypeImage:
+    return processImageType(operands);
   case spirv::Opcode::OpTypeRuntimeArray:
     return processRuntimeArrayType(operands);
   case spirv::Opcode::OpTypeStruct:
     return processStructType(operands);
   case spirv::Opcode::OpTypeMatrix:
     return processMatrixType(operands);
   default:
     return emitError(unknownLoc, "unhandled type instruction");
spirv::Deserializer::processTypeForwardPointer(ArrayRef<uint32_t> operands) {
 
   typeForwardPointerIDs.insert(operands[0]);
   // TODO: Use the 2nd operand (Storage Class) to validate the OpTypePointer
   // instruction that defines the actual type.
 
   return success();
 }
 
+LogicalResult
+spirv::Deserializer::processImageType(ArrayRef<uint32_t> operands) {
+  // TODO: Add support for Access Qualifier.
+  if (operands.size() != 8)
+    return emitError(
+        unknownLoc,
+        "OpTypeImage with non-eight operands are not supported yet");
+
+  Type elementTy = getType(operands[1]);
+  if (!elementTy)
+    return emitError(unknownLoc, "OpTypeImage references undefined <id>: ")
+           << operands[1];
+
+  auto dim = spirv::symbolizeDim(operands[2]);
+  if (!dim)
+    return emitError(unknownLoc, "unknown Dim for OpTypeImage: ")
+           << operands[2];
+
+  auto depthInfo = spirv::symbolizeImageDepthInfo(operands[3]);
+  if (!depthInfo)
+    return emitError(unknownLoc, "unknown Depth for OpTypeImage: ")
+           << operands[3];
+
+  auto arrayedInfo = spirv::symbolizeImageArrayedInfo(operands[4]);
+  if (!arrayedInfo)
+    return emitError(unknownLoc, "unknown Arrayed for OpTypeImage: ")
+           << operands[4];
+
+  auto samplingInfo = spirv::symbolizeImageSamplingInfo(operands[5]);
+  if (!samplingInfo)
+    return emitError(unknownLoc, "unknown MS for OpTypeImage: ") << operands[5];
+
+  auto samplerUseInfo = spirv::symbolizeImageSamplerUseInfo(operands[6]);
+  if (!samplerUseInfo)
+    return emitError(unknownLoc, "unknown Sampled for OpTypeImage: ")
+           << operands[6];
+
+  auto format = spirv::symbolizeImageFormat(operands[7]);
+  if (!format)
+    return emitError(unknownLoc, "unknown Format for OpTypeImage: ")
+           << operands[7];
+
+  typeMap[operands[0]] = spirv::ImageType::get(
+      elementTy, dim.getValue(), depthInfo.getValue(), arrayedInfo.getValue(),
+      samplingInfo.getValue(), samplerUseInfo.getValue(), format.getValue());
+  return success();
+}
+
 //===----------------------------------------------------------------------===//
 // Constant
 //===----------------------------------------------------------------------===//
 
 LogicalResult spirv::Deserializer::processConstant(ArrayRef<uint32_t> operands,
                                                    bool isSpec) {
   StringRef opname = isSpec ? "OpSpecConstant" : "OpConstant";
 

mlir/lib/Target/SPIRV/Serialization/Serialization.cpp

if (failed(processTypeImpl(loc, vectorType.getElementType(), elementTypeID,
       return failure();
     }
     typeEnum = spirv::Opcode::OpTypeVector;
     operands.push_back(elementTypeID);
     operands.push_back(vectorType.getNumElements());
     return success();
   }
 
+  if (auto imageType = type.dyn_cast<spirv::ImageType>()) {
+    typeEnum = spirv::Opcode::OpTypeImage;
+    uint32_t sampledTypeID = 0;
+    if (failed(processType(loc, imageType.getElementType(), sampledTypeID)))
+      return failure();
+
+    operands.push_back(sampledTypeID);
+    operands.push_back(static_cast<uint32_t>(imageType.getDim()));
+    operands.push_back(static_cast<uint32_t>(imageType.getDepthInfo()));
+    operands.push_back(static_cast<uint32_t>(imageType.getArrayedInfo()));
+    operands.push_back(static_cast<uint32_t>(imageType.getSamplingInfo()));
+    operands.push_back(static_cast<uint32_t>(imageType.getSamplerUseInfo()));
+    operands.push_back(static_cast<uint32_t>(imageType.getImageFormat()));
+    return success();
+  }
+
   if (auto arrayType = type.dyn_cast<spirv::ArrayType>()) {
     typeEnum = spirv::Opcode::OpTypeArray;
     uint32_t elementTypeID = 0;
     if (failed(processTypeImpl(loc, arrayType.getElementType(), elementTypeID,
                                serializationCtx))) {
       return failure();
     }
     operands.push_back(elementTypeID);

mlir/test/Target/SPIRV/image.mlir

+// RUN: mlir-translate -test-spirv-roundtrip %s | FileCheck %s
+
+spv.module Logical GLSL450 requires #spv.vce<v1.0, [Shader], []> {
+  // CHECK: !spv.ptr<!spv.image<f32, Dim1D, NoDepth, NonArrayed, SingleSampled, SamplerUnknown, Unknown>, UniformConstant>
+  spv.globalVariable @var0 bind(0, 1) : !spv.ptr<!spv.image<f32, Dim1D, NoDepth, NonArrayed, SingleSampled, SamplerUnknown, Unknown>, UniformConstant>
+
+  // CHECK: !spv.ptr<!spv.image<si32, Cube, IsDepth, NonArrayed, SingleSampled, NeedSampler, R8ui>, UniformConstant>
+  spv.globalVariable @var1 : !spv.ptr<!spv.image<si32, Cube, IsDepth, NonArrayed, SingleSampled, NeedSampler, R8ui>, UniformConstant>
+
+  // CHECK: !spv.ptr<!spv.image<i32, SubpassData, DepthUnknown, Arrayed, MultiSampled, NoSampler, Unknown>, UniformConstant>
+  spv.globalVariable @var2 : !spv.ptr<!spv.image<i32, SubpassData, DepthUnknown, Arrayed, MultiSampled, NoSampler, Unknown>, UniformConstant>
+}