[mlir:PDLL] Don't require users to provide operands/results when all are variadic

[rriddle]

Summary

When all operands or results are variadic, zero values is a perfectly valid behavior
to expect, and we shouldn't force the user to provide values in this case. For example,
when creating a call or a return operation we often don't want/need to provide return
values.

[jpienaar]

mlir/test/lib/Transforms/TestDialectConversion.pdll

13–14

This is more concise, and especially can see how this is better with multiple optional and not too magical. The previous form is indeed closer to ODS.

mlir/lib/Tools/PDLL/Parser/Parser.cpp

private:
 
   /// Validate the member access `name` into the given parent expression. On
   /// success, this also returns the type of the member accessed.
   FailureOr<ast::Type> validateMemberAccess(ast::Expr *parentExpr,
                                             StringRef name, SMRange loc);
   FailureOr<ast::OperationExpr *>
   createOperationExpr(SMRange loc, const ast::OpNameDecl *name,
                       OpResultTypeContext resultTypeContext,
-                      MutableArrayRef<ast::Expr *> operands,
+                      SmallVectorImpl<ast::Expr *> &operands,
                       MutableArrayRef<ast::NamedAttributeDecl *> attributes,
-                      MutableArrayRef<ast::Expr *> results);
+                      SmallVectorImpl<ast::Expr *> &results);
   LogicalResult
   validateOperationOperands(SMRange loc, Optional<StringRef> name,
                             const ods::Operation *odsOp,
-                            MutableArrayRef<ast::Expr *> operands);
+                            SmallVectorImpl<ast::Expr *> &operands);
   LogicalResult validateOperationResults(SMRange loc, Optional<StringRef> name,
                                          const ods::Operation *odsOp,
-                                         MutableArrayRef<ast::Expr *> results);
+                                         SmallVectorImpl<ast::Expr *> &results);
   void checkOperationResultTypeInferrence(SMRange loc, StringRef name,
                                           const ods::Operation *odsOp);
   LogicalResult validateOperationOperandsOrResults(
       StringRef groupName, SMRange loc, Optional<SMRange> odsOpLoc,
-      Optional<StringRef> name, MutableArrayRef<ast::Expr *> values,
+      Optional<StringRef> name, SmallVectorImpl<ast::Expr *> &values,
       ArrayRef<ods::OperandOrResult> odsValues, ast::Type singleTy,
-      ast::Type rangeTy);
+      ast::RangeType rangeTy);
   FailureOr<ast::TupleExpr *> createTupleExpr(SMRange loc,
                                               ArrayRef<ast::Expr *> elements,
                                               ArrayRef<StringRef> elementNames);
 
   //===--------------------------------------------------------------------===//
   // Stmts
 
   FailureOr<ast::EraseStmt *> createEraseStmt(SMRange loc, ast::Expr *rootOp);
return emitError(
       loc,
       llvm::formatv("invalid member access `{0}` on expression of type `{1}`",
                     name, parentType));
 }
 
 FailureOr<ast::OperationExpr *> Parser::createOperationExpr(
     SMRange loc, const ast::OpNameDecl *name,
     OpResultTypeContext resultTypeContext,
-    MutableArrayRef<ast::Expr *> operands,
+    SmallVectorImpl<ast::Expr *> &operands,
     MutableArrayRef<ast::NamedAttributeDecl *> attributes,
-    MutableArrayRef<ast::Expr *> results) {
+    SmallVectorImpl<ast::Expr *> &results) {
   Optional<StringRef> opNameRef = name->getName();
   const ods::Operation *odsOp = lookupODSOperation(opNameRef);
 
   // Verify the inputs operands.
   if (failed(validateOperationOperands(loc, opNameRef, odsOp, operands)))
     return failure();
 
   // Verify the attribute list.
FailureOr<ast::OperationExpr *> Parser::createOperationExpr(
 
   return ast::OperationExpr::create(ctx, loc, odsOp, name, operands, results,
                                     attributes);
 }
 
 LogicalResult
 Parser::validateOperationOperands(SMRange loc, Optional<StringRef> name,
                                   const ods::Operation *odsOp,
-                                  MutableArrayRef<ast::Expr *> operands) {
+                                  SmallVectorImpl<ast::Expr *> &operands) {
   return validateOperationOperandsOrResults(
       "operand", loc, odsOp ? odsOp->getLoc() : Optional<SMRange>(), name,
       operands, odsOp ? odsOp->getOperands() : llvm::None, valueTy,
       valueRangeTy);
 }
 
 LogicalResult
 Parser::validateOperationResults(SMRange loc, Optional<StringRef> name,
                                  const ods::Operation *odsOp,
-                                 MutableArrayRef<ast::Expr *> results) {
+                                 SmallVectorImpl<ast::Expr *> &results) {
   return validateOperationOperandsOrResults(
       "result", loc, odsOp ? odsOp->getLoc() : Optional<SMRange>(), name,
       results, odsOp ? odsOp->getResults() : llvm::None, typeTy, typeRangeTy);
 }
 
 void Parser::checkOperationResultTypeInferrence(SMRange loc, StringRef opName,
                                                 const ods::Operation *odsOp) {
   // If the operation might not have inferrence support, emit a warning to the
if (requiresInferrence && !odsOp->hasResultTypeInferrence()) {
     diag->attachNote(llvm::formatv("see the definition of `{0}` here", opName),
                      odsOp->getLoc());
     return;
   }
 }
 
 LogicalResult Parser::validateOperationOperandsOrResults(
     StringRef groupName, SMRange loc, Optional<SMRange> odsOpLoc,
-    Optional<StringRef> name, MutableArrayRef<ast::Expr *> values,
+    Optional<StringRef> name, SmallVectorImpl<ast::Expr *> &values,
     ArrayRef<ods::OperandOrResult> odsValues, ast::Type singleTy,
-    ast::Type rangeTy) {
+    ast::RangeType rangeTy) {
   // All operation types accept a single range parameter.
   if (values.size() == 1) {
     if (failed(convertExpressionTo(values[0], rangeTy)))
       return failure();
     return success();
   }
 
   /// If the operation has ODS information, we can more accurately verify the
   /// values.
   if (odsOpLoc) {
-    if (odsValues.size() != values.size()) {
+    auto emitSizeMismatchError = [&] {
       return emitErrorAndNote(
           loc,
           llvm::formatv("invalid number of {0} groups for `{1}`; expected "
                         "{2}, but got {3}",
                         groupName, *name, odsValues.size(), values.size()),
           *odsOpLoc, llvm::formatv("see the definition of `{0}` here", *name));
+    };
+
+    // Handle the case where no values were provided.
+    if (values.empty()) {
+      // If we don't expect any on the ODS side, we are done.
+      if (odsValues.empty())
+        return success();
+
+      // If we do, check if we actually need to provide values (i.e. if any of
+      // the values are actually required).
+      unsigned numVariadic = 0;
+      for (const auto &odsValue : odsValues) {
+        if (!odsValue.isVariableLength())
+          return emitSizeMismatchError();
+        ++numVariadic;
       }
+
+      // If we are in a non-rewrite context, we don't need to do anything more.
+      // Zero-values is a valid constraint on the operation.
+      if (parserContext != ParserContext::Rewrite)
+        return success();
+
+      // Otherwise, when in a rewrite we may need to provide values to match the
+      // ODS signature of the operation to create.
+
+      // If we only have one variadic value, just use an empty list.
+      if (numVariadic == 1)
+        return success();
+
+      // Otherwise, create dummy values for each of the entries so that we
+      // adhere to the ODS signature.
+      for (unsigned i = 0, e = odsValues.size(); i < e; ++i) {
+        values.push_back(
+            ast::RangeExpr::create(ctx, loc, /*elements=*/llvm::None, rangeTy));
+      }
+      return success();
+    }
+
+    // Verify that the number of values provided matches the number of value
+    // groups ODS expects.
+    if (odsValues.size() != values.size())
+      return emitSizeMismatchError();
+
     auto diagFn = [&](ast::Diagnostic &diag) {
       diag.attachNote(llvm::formatv("see the definition of `{0}` here", *name),
                       *odsOpLoc);
     };
     for (unsigned i = 0, e = values.size(); i < e; ++i) {
       ast::Type expectedType = odsValues[i].isVariadic() ? rangeTy : singleTy;
       if (failed(convertExpressionTo(values[i], expectedType, diagFn)))
         return failure();

mlir/test/lib/Transforms/TestDialectConversion.pdll

 //===- TestPDLL.pdll - Test PDLL functionality ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "TestOps.td"
 #include "mlir/Transforms/DialectConversion.pdll"
 
 /// Change the result type of a producer.
-// FIXME: We shouldn't need to specify arguments for the result cast.
-Pattern => replace op<test.cast>(args: ValueRange) -> (results: TypeRange)
-  with op<test.cast>(args) -> (convertTypes(results));
+Pattern => replace op<test.cast> -> (results: TypeRange)
+  with op<test.cast> -> (convertTypes(results));
 
 /// Pass through test.return conversion.
 Pattern => replace op<test.return>(args: ValueRange)
   with op<test.return>(convertValues(args));

mlir/test/mlir-pdll/Parser/expr.pdll

 // CHECK:     |-DeclRefExpr {{.*}} Type<TypeRange>
 // CHECK:     `-DeclRefExpr {{.*}} Type<Type>
 Pattern {
   erase op<> -> (_: Type, _: TypeRange, _: Type);
 }
 
 // -----
 
+// Test that we don't need to provide values if all elements
+// are optional.
+
+#include "include/ops.td"
+
+// CHECK: Module
+// CHECK:  -OperationExpr {{.*}} Type<Op<test.multi_variadic>>
+// CHECK-NOT:   `Operands`
+// CHECK-NOT:   `Result Types`
+// CHECK:  -OperationExpr {{.*}} Type<Op<test.all_variadic>>
+// CHECK-NOT:   `Operands`
+// CHECK-NOT:   `Result Types`
+// CHECK:  -OperationExpr {{.*}} Type<Op<test.multi_variadic>>
+// CHECK:    `Operands`
+// CHECK:      -RangeExpr {{.*}} Type<ValueRange>
+// CHECK:      -RangeExpr {{.*}} Type<ValueRange>
+// CHECK:    `Result Types`
+// CHECK:      -RangeExpr {{.*}} Type<TypeRange>
+// CHECK:      -RangeExpr {{.*}} Type<TypeRange>
+Pattern {
+  rewrite op<test.multi_variadic>() -> () with {
+    op<test.all_variadic> -> ();
+    op<test.multi_variadic> -> ();
+  };
+}
+
+// -----
+
 //===----------------------------------------------------------------------===//
 // TupleExpr
 //===----------------------------------------------------------------------===//
 
 // CHECK: Module
 // CHECK: `-VariableDecl {{.*}} Name<emptyTuple>
 // CHECK:   `-TupleExpr {{.*}} Type<Tuple<>>
 // CHECK: `-VariableDecl {{.*}} Name<mixedTuple>

mlir/test/mlir-pdll/Parser/include/ops.td

 def OpAllVariadic : Op<Test_Dialect, "all_variadic"> {
   let arguments = (ins Variadic<I64>:$operands);
   let results = (outs Variadic<I64>:$results);
 }
 
 def OpMultipleSingleResult : Op<Test_Dialect, "multiple_single_result"> {
   let results = (outs I64:$result, I64:$result2);
 }
+
+def OpMultiVariadic : Op<Test_Dialect, "multi_variadic"> {
+  let arguments = (ins Variadic<I64>:$operands, Variadic<I64>:$operand2);
+  let results = (outs Variadic<I64>:$results, Variadic<I64>:$results2);
+}