[mlir][spirv] Enhance AccessChainOp index type handling

[Hazem]

Summary

This patch extends the AccessChainOp index type handling to be able to deal with all Integer type indices (i.e., all bit-widths and signedness symantics).

There were two ways of achieving this:
1- Backward compatible: The new way of handling the indices will assume that an index type is i32 by default if not specified in the assembly format, this way all the old tests would pass correctly.
2- Enforce the format: This unifies the spv.AccessChain Op format and all the old tests had to be updated to reflect this change or else they fail.

I picked option-2 to unify the Op format and avoid having optional index-type fields that can lead to somewhat confusing tests format and multiple representations for the same Op with undocumented assumption that an index is i32 unless stated. Nonetheless, reverting to option-1 should be straightforward if
preferred or needed.

[rriddle]

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

801

nit: diagnostics implicitly convert to failure.

return emitError(...)

812

Same here.

832

nit: printer << op.indices().getTypes()

[antiagainst]

Cool! Looks good to me; just one more nit.

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

811

I think only indicesTypes.size() != indicesInfo.size() will suffice here?

[Hazem]

• address revision comments

[Hazem]

Does anyone know why the pre-merge test fails? It is pointing out to line "mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:798" and it is not clear to me from the diagnostics what is the problem that needs to be fixed. Thanks!

[antiagainst]

In D81763#2099265, @Hazem wrote:

Does anyone know why the pre-merge test fails? It is pointing out to line "mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:798" and it is not clear to me from the diagnostics what is the problem that needs to be fixed. Thanks!

Did you accidentally reverted the majority of the changes in this patch? I'm only seeing changes to SPIRVOps.cpp now..

[Hazem]

• Fix unintentional files remove from patch

[Hazem]

I fixed it now. Thanks!

In D81763#2099410, @antiagainst wrote:

In D81763#2099265, @Hazem wrote:

Does anyone know why the pre-merge test fails? It is pointing out to line "mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:798" and it is not clear to me from the diagnostics what is the problem that needs to be fixed. Thanks!

Did you accidentally reverted the majority of the changes in this patch? I'm only seeing changes to SPIRVOps.cpp now..

[Hazem]

Hello @antiagainst

After submitting this patch, do you know how can I update the documentation? (https://mlir.llvm.org/docs/Dialects/SPIR-V/#spvaccesschain-spirvaccesschainop)

In D81763#2099701, @Hazem wrote:

I fixed it now. Thanks!

In D81763#2099410, @antiagainst wrote:

In D81763#2099265, @Hazem wrote:

Does anyone know why the pre-merge test fails? It is pointing out to line "mlir/lib/Dialect/SPIRV/SPIRVOps.cpp:798" and it is not clear to me from the diagnostics what is the problem that needs to be fixed. Thanks!

Did you accidentally reverted the majority of the changes in this patch? I'm only seeing changes to SPIRVOps.cpp now..

[antiagainst]

Thanks @Hazem!

In D81763#2104138, @Hazem wrote:

Hello @antiagainst

After submitting this patch, do you know how can I update the documentation? (https://mlir.llvm.org/docs/Dialects/SPIR-V/#spvaccesschain-spirvaccesschainop)

The documentation is autogenerated (see https://github.com/llvm/mlir-www for more details). So don't worry about it. :)

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp

if (parser.parseOperand(ptrInfo) ||
       parser.parseColonType(type) ||
       parser.resolveOperand(ptrInfo, type, state.operands)) {
     return failure();
   }
 
   // Check that the provided indices list is not empty before parsing their
   // type list.
   if (indicesInfo.empty()) {
-    emitError(state.location, "'spv.AccessChain' op expected at least "
-                              "one index ");
-    return failure();
+    return emitError(state.location, "'spv.AccessChain' op expected at "
+                                     "least one index ");
   }
 
   if (parser.parseComma() || parser.parseTypeList(indicesTypes))
     return failure();
 
   // Check that the indices types list is not empty and that it has a one-to-one
   // mapping to the provided indices.
-  if (indicesTypes.empty() || indicesTypes.size() != indicesInfo.size()) {
-    emitError(state.location, "'spv.AccessChain' op indices types' count"
-                              " must be equal to indices info count");
-    return failure();
+  if (indicesTypes.size() != indicesInfo.size()) {
+    return emitError(state.location, "'spv.AccessChain' op indices "
+                                     "types' count must be equal to indices "
+                                     "info count");
   }
 
   if (parser.resolveOperands(indicesInfo, indicesTypes, loc, state.operands))
     return failure();
 
   auto resultType = getElementPtrType(
       type, llvm::makeArrayRef(state.operands).drop_front(), state.location);
   if (!resultType) {
     return failure();
   }
 
   state.addTypes(resultType);
   return success();
 }
 
 static void print(spirv::AccessChainOp op, OpAsmPrinter &printer) {
   printer << spirv::AccessChainOp::getOperationName() << ' ' << op.base_ptr()
-          << '[' << op.indices() << "] : " << op.base_ptr().getType();
-
-  for (auto index : op.indices()) {
-    printer << ", " << index.getType();
-  }
+          << '[' << op.indices() << "] : " << op.base_ptr().getType() << ", "
+          << op.indices().getTypes();
 }
 
 static LogicalResult verify(spirv::AccessChainOp accessChainOp) {
   SmallVector<Value, 4> indices(accessChainOp.indices().begin(),
                                 accessChainOp.indices().end());
   auto resultType = getElementPtrType(accessChainOp.base_ptr().getType(),
                                       indices, accessChainOp.getLoc());
   if (!resultType) {