Fix review comments

Signed-off-by: Davide Grohmann <[email protected]>
Change-Id: Ib592a4b99af01c0d3c88eaf63a61cb4c6cca8cbe

Author: davidegrohmann

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

@@ -51,7 +51,7 @@ def SPIRV_GraphARMOp : SPIRV_GraphARMOp<"Graph", [
     that is unique in the enclosing module op.
 
     This op itself takes no operands and generates no results. Its region
-    can take zero or more arguments and return zero or more values.
+    can take zero or more arguments and return one or more values.
 
     ```
     spv-graph-arm-op ::= `spirv.ARM.Graph` function-signature
@@ -119,10 +119,6 @@ def SPIRV_GraphConstantARMOp : SPIRV_GraphARMOp<"GraphConstant", [InGraphScope,
     Result Type must be an OpTypeTensorARM.
     GraphConstantID must be a 32-bit integer literal.
 
-    ```
-    spv-graph-constant-arm-op ::= `spirv.ARM.GraphConstant` { graph_constant_id = 42 : i32 }
-    ```
-
     #### Example:
 
     ```mlir
@@ -167,11 +163,6 @@ def SPIRV_GraphEntryPointARMOp : SPIRV_GraphARMOp<"GraphEntryPoint", [InModuleSc
     `spirv.GlobalVariable`s referenced by the entry point’s static call
     tree, within the interface’s storage classes.
 
-    ```
-    entry-point-op ::= ssa-id `=` `spirv.ARM.GraphEntryPoint`
-                       symbol-reference (`, ` symbol-reference)*
-    ```
-
     #### Example:
 
     ```mlir
@@ -214,10 +205,6 @@ def SPIRV_GraphOutputsARMOp : SPIRV_GraphARMOp<"GraphOutputs", [InGraphScope, Pu
 
     This instruction must be the last instruction in a block.
 
-    ```
-    graph-output-op ::= `spirv.ARM.GraphOutputs` ssa-use `:` type-list-no-parens
-    ```
-
     #### Example:
 
     ```mlir

mlir/lib/Dialect/SPIRV/IR/ArmGraphOps.cpp

@@ -1,5 +1,4 @@
-//===- ArmGraphOps.cpp - MLIR SPIR-V SPV_ARM_graph operations
-//------------------------------===//
+//===- ArmGraphOps.cpp - MLIR SPIR-V SPV_ARM_graph operations -------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -22,6 +21,7 @@
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Interfaces/FunctionImplementation.h"
+#include "llvm/Support/InterleavedRange.h"
 
 using namespace mlir;
 using namespace mlir::spirv::AttrNames;
@@ -32,10 +32,7 @@ using namespace mlir::spirv::AttrNames;
 
 ParseResult spirv::GraphARMOp::parse(OpAsmParser &parser,
                                      OperationState &result) {
-  SmallVector<OpAsmParser::Argument> entryArgs;
-  SmallVector<DictionaryAttr> resultAttrs;
-  SmallVector<Type> resultTypes;
-  auto &builder = parser.getBuilder();
+  Builder &builder = parser.getBuilder();
 
   // Parse the name as a symbol.
   StringAttr nameAttr;
@@ -45,15 +42,18 @@ ParseResult spirv::GraphARMOp::parse(OpAsmParser &parser,
 
   // Parse the function signature.
   bool isVariadic = false;
+  SmallVector<OpAsmParser::Argument> entryArgs;
+  SmallVector<Type> resultTypes;
+  SmallVector<DictionaryAttr> resultAttrs;
   if (function_interface_impl::parseFunctionSignatureWithArguments(
           parser, /*allowVariadic=*/false, entryArgs, isVariadic, resultTypes,
           resultAttrs))
     return failure();
 
   SmallVector<Type> argTypes;
-  for (auto &arg : entryArgs)
+  for (OpAsmParser::Argument &arg : entryArgs)
     argTypes.push_back(arg.type);
-  auto grType = builder.getGraphType(argTypes, resultTypes);
+  GraphType grType = builder.getGraphType(argTypes, resultTypes);
   result.addAttribute(getFunctionTypeAttrName(result.name),
                       TypeAttr::get(grType));
 
@@ -136,26 +136,22 @@ LogicalResult spirv::GraphARMOp::verifyBody() {
   }
 
   GraphType grType = getFunctionType();
-  auto walkResult = walk([grType](Operation *op) -> WalkResult {
-    if (auto graphOutputsARMOp = dyn_cast<spirv::GraphOutputsARMOp>(op)) {
-      if (grType.getNumResults() != graphOutputsARMOp.getNumOperands())
-        return graphOutputsARMOp.emitOpError("is returning ")
-               << graphOutputsARMOp.getNumOperands()
-               << " value(s) but enclosing spirv.ARM.Graph requires "
-               << grType.getNumResults() << " result(s)";
-
-      ValueTypeRange<OperandRange> graphOutputOperandTypes =
-          graphOutputsARMOp.getValue().getType();
-      for (unsigned i = 0, size = graphOutputOperandTypes.size(); i < size;
-           ++i) {
-        Type graphOutputOperandType = graphOutputOperandTypes[i];
-        Type grResultType = grType.getResult(i);
-        if (graphOutputOperandType != grResultType)
-          return graphOutputsARMOp.emitError("type of return operand ")
-                 << i << " (" << graphOutputOperandType
-                 << ") doesn't match graph result type (" << grResultType
-                 << ")";
-      }
+  auto walkResult = walk([grType](spirv::GraphOutputsARMOp op) -> WalkResult {
+    if (grType.getNumResults() != op.getNumOperands())
+      return op.emitOpError("is returning ")
+             << op.getNumOperands()
+             << " value(s) but enclosing spirv.ARM.Graph requires "
+             << grType.getNumResults() << " result(s)";
+
+    ValueTypeRange<OperandRange> graphOutputOperandTypes =
+        op.getValue().getType();
+    for (unsigned i = 0, size = graphOutputOperandTypes.size(); i < size; ++i) {
+      Type graphOutputOperandType = graphOutputOperandTypes[i];
+      Type grResultType = grType.getResult(i);
+      if (graphOutputOperandType != grResultType)
+        return op.emitError("type of return operand ")
+               << i << " (" << graphOutputOperandType
+               << ") doesn't match graph result type (" << grResultType << ")";
     }
     return WalkResult::advance();
   });
@@ -169,23 +165,20 @@ void spirv::GraphARMOp::build(OpBuilder &builder, OperationState &state,
   state.addAttribute(SymbolTable::getSymbolAttrName(),
                      builder.getStringAttr(name));
   state.addAttribute(getFunctionTypeAttrName(state.name), TypeAttr::get(type));
-  state.attributes.append(attrs.begin(), attrs.end());
+  state.attributes.append(attrs);
   state.addAttribute(getEntryPointAttrName(state.name),
                      builder.getBoolAttr(entryPoint));
   state.addRegion();
 }
 
-// Returns the argument types of this function.
 ArrayRef<Type> spirv::GraphARMOp::getArgumentTypes() {
   return getFunctionType().getInputs();
 }
 
-// Returns the result types of this function.
 ArrayRef<Type> spirv::GraphARMOp::getResultTypes() {
   return getFunctionType().getResults();
 }
 
-// CallableOpInterface
 Region *spirv::GraphARMOp::getCallableRegion() {
   return isExternal() ? nullptr : &getBody();
 }
@@ -229,12 +222,11 @@ void spirv::GraphEntryPointARMOp::build(OpBuilder &builder,
 
 ParseResult spirv::GraphEntryPointARMOp::parse(OpAsmParser &parser,
                                                OperationState &result) {
-  SmallVector<Attribute, 4> interfaceVars;
-
   FlatSymbolRefAttr fn;
   if (parser.parseAttribute(fn, Type(), kFnNameAttrName, result.attributes))
     return failure();
 
+  SmallVector<Attribute, 4> interfaceVars;
   if (!parser.parseOptionalComma()) {
     // Parse the interface variables
     if (parser.parseCommaSeparatedList([&]() -> ParseResult {
@@ -258,7 +250,6 @@ void spirv::GraphEntryPointARMOp::print(OpAsmPrinter &printer) {
   printer.printSymbolName(getFn());
   ArrayRef<Attribute> interfaceVars = getInterface().getValue();
   if (!interfaceVars.empty()) {
-    printer << ", ";
-    llvm::interleaveComma(interfaceVars, printer);
+    printer << ", " << llvm::interleaved(interfaceVars);
   }
 }

mlir/lib/Dialect/SPIRV/Transforms/UpdateVCEPass.cpp

@@ -160,10 +160,8 @@ void UpdateVCEPass::runOnOperation() {
 
     // If the op is FunctionLike make sure to process input and result types
     if (auto funcOpInterface = dyn_cast<FunctionOpInterface>(op)) {
-      ArrayRef<Type> inputTypes = funcOpInterface.getArgumentTypes();
-      ArrayRef<Type> resultTypes = funcOpInterface.getResultTypes();
-      valueTypes.append(inputTypes.begin(), inputTypes.end());
-      valueTypes.append(resultTypes.begin(), resultTypes.end());
+      llvm::append_range(valueTypes, funcOpInterface.getArgumentTypes());
+      llvm::append_range(valueTypes, funcOpInterface.getResultTypes());
     }
 
     // Requirements from values' types

mlir/lib/IR/AsmPrinter.cpp

@@ -104,8 +104,7 @@ void OpAsmPrinter::printFunctionalType(Operation *op) {
   // it is a function (avoiding a grammar ambiguity).
   bool wrapped = op->getNumResults() != 1;
   if (!wrapped && op->getResult(0).getType() &&
-      (llvm::isa<FunctionType>(op->getResult(0).getType()) ||
-       llvm::isa<GraphType>(op->getResult(0).getType())))
+      isa<GraphType>(op->getResult(0).getType()))
     wrapped = true;
 
   if (wrapped)
@@ -2842,8 +2841,8 @@ void AsmPrinter::Impl::printTypeImpl(Type type) {
         interleaveComma(graphTy.getInputs(), [&](Type ty) { printType(ty); });
         os << ") -> ";
         ArrayRef<Type> results = graphTy.getResults();
-        if (results.size() == 1 && !(llvm::isa<FunctionType>(results[0]) ||
-                                     llvm::isa<GraphType>(results[0]))) {
+        if (results.size() == 1 &&
+            !(isa<FunctionType>(results[0]) || isa<GraphType>(results[0]))) {
           printType(results[0]);
         } else {
           os << '(';

mlir/test/Dialect/SPIRV/IR/graph-ops.mlir

@@ -1,4 +1,4 @@
-// RUN: mlir-opt -split-input-file -verify-diagnostics %s | FileCheck %s
+// RUN: mlir-opt --split-input-file --verify-diagnostics %s | FileCheck %s
 
 //===----------------------------------------------------------------------===//
 // spirv.ARM.Graph and spirv.ARM.GraphOutputs

mlir/test/lib/Dialect/SPIRV/TestAvailability.cpp

@@ -33,7 +33,7 @@ struct PrintOpAvailability
 } // namespace
 
 void PrintOpAvailability::runOnOperation() {
-  auto moduleOp = getOperation();
+  mlir::ModuleOp moduleOp = getOperation();
   Dialect *spirvDialect = getContext().getLoadedDialect("spirv");
 
   auto opCallback = [&](Operation *op) {