[mlir][python] Swap operands/results order on default builder.

[stellaraccident]

Summary

• I've used this a lot and the order we inherit from C++ is likely to bite us. It will also be very disruptive to change later, so better to do now.
• Everywhere else in the Python API, we order arguments in order of increasing optionality, as that is what lets you produce most the nicest signatures. When I've been writing custom builders, often the results can be inferred or otherwise ommitted to produce the most usable form, but then the default builder always builds [results, operands]. This patch inverts that to match elsewhere.
• Also fixes a couple of other signature nits (making the trailing loc/ip arguments keyword only, and properly naming unnamed unary results to match the default definitions).
• Removes the prohibited word 'result' in names now that it is handled correctly.

[ftynse]

Not sure about this one... I can understand the rationale, but this starts to diverge from C++/ODS, which already has suboptimal documentation and I'm apprehensive of the complexity it might add. If we agree we want to put result types after operands, we should be doing so consistently and that could be a problem for generating builders automatically if we decide we want that.

mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp

648

What are the extra args we accept?

[jpienaar]

In D94810#2504721, @ftynse wrote:

Not sure about this one... I can understand the rationale, but this starts to diverge from C++/ODS <snip irrelevant> . If we agree we want to put result types after operands, we should be doing so consistently

If we can infer the type, we already generate different builders in C++. My 2c: I don't think it is worth migration there (one could argue about what is more common: having operands or being able to infer return type). Here I could see it for the more "user facing" parts, but for those we'd want to generate custom builders + not allow specifying type when you'll be inferring it (make it more difficult to create invalid ops). Now if you have set of ops where you can all infer their types, then you have consistently no return type(s) arg, else you have a mix but I can't see how one could avoid the mix.

[stellaraccident]

In D94810#2504721, @ftynse wrote:

Not sure about this one... I can understand the rationale, but this starts to diverge from C++/ODS, which already has suboptimal documentation and I'm apprehensive of the complexity it might add. If we agree we want to put result types after operands, we should be doing so consistently and that could be a problem for generating builders automatically if we decide we want that.

When I was writing this, I was borderline as to whether it should be a thread of just a patch. Let me start a thread.

[stellaraccident]

Rebase and fork from parents.

[stellaraccident]

Forked to discourse: https://llvm.discourse.group/t/finalizing-operand-result-ordering-in-python-bindings/2612

[stellaraccident]

As discussed on discourse, we won't be going forward with this change. I've forked the things that survive in to D95320 and will do another patch to implement the "most specific builder" approach that jpienaar pointed out.

mlir/test/Bindings/Python/dialects.py

with Context() as ctx, Location.unknown():
     ctx.allow_unregistered_dialects = True
     m = Module.create()
 
     with InsertionPoint.at_block_terminator(m.body):
       f32 = F32Type.get()
       # Create via dialects context collection.
       input1 = createInput()
       input2 = createInput()
-      op1 = ctx.dialects.std.AddFOp(input1.type, input1, input2)
+      op1 = ctx.dialects.std.AddFOp(input1, input2, result=input1.type)
 
       # Create via an import
       from mlir.dialects.std import AddFOp
-      AddFOp(input1.type, input1, op1.result)
+      AddFOp(input1, op1.result, input1.type)
 
   # CHECK: %[[INPUT0:.*]] = "pytest_dummy.intinput"
   # CHECK: %[[INPUT1:.*]] = "pytest_dummy.intinput"
   # CHECK: %[[R0:.*]] = addf %[[INPUT0]], %[[INPUT1]] : f32
   # CHECK: %[[R1:.*]] = addf %[[INPUT0]], %[[R0]] : f32
   m.operation.print()
 
 
 run(testCustomOpView)

mlir/test/mlir-tblgen/op-python-bindings.td

class TestOp<string mnemonic, list<OpTrait> traits = []> :
     Op<Test_Dialect, mnemonic, traits>;
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class AttrSizedOperandsOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.attr_sized_operands"
 // CHECK: _ODS_OPERAND_SEGMENTS = [-1,1,-1,]
 def AttrSizedOperandsOp : TestOp<"attr_sized_operands",
                                  [AttrSizedOperandSegments]> {
-  // CHECK: def __init__(self, variadic1, non_variadic, variadic2, loc=None, ip=None):
+  // CHECK: def __init__(self, variadic1, non_variadic, variadic2, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   operands.append(variadic1)
   // CHECK:   operands.append(non_variadic)
   // CHECK:   if variadic2 is not None: operands.append(variadic2)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,

 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class AttrSizedResultsOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.attr_sized_results"
 // CHECK: _ODS_RESULT_SEGMENTS = [-1,1,-1,]
 def AttrSizedResultsOp : TestOp<"attr_sized_results",
                                [AttrSizedResultSegments]> {
-  // CHECK: def __init__(self, variadic1, non_variadic, variadic2, loc=None, ip=None):
+  // CHECK: def __init__(self, variadic1, non_variadic, variadic2, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   if variadic1 is not None: results.append(variadic1)
   // CHECK:   results.append(non_variadic)
   // CHECK:   if variadic2 is not None: results.append(variadic2)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,

 
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class AttributedOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.attributed_op"
 // CHECK-NOT: _ODS_OPERAND_SEGMENTS
 // CHECK-NOT: _ODS_RESULT_SEGMENTS
 def AttributedOp : TestOp<"attributed_op"> {
-  // CHECK: def __init__(self, i32attr, optionalF32Attr, unitAttr, in_, loc=None, ip=None):
+  // CHECK: def __init__(self, i32attr, optionalF32Attr, unitAttr, in_, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   attributes["i32attr"] = i32attr
   // CHECK:   if optionalF32Attr is not None: attributes["optionalF32Attr"] = optionalF32Attr
   // CHECK:   if bool(unitAttr): attributes["unitAttr"] = _ods_ir.UnitAttr.get(
   // CHECK:     _ods_get_default_loc_context(loc))
   // CHECK:   attributes["in"] = in_

 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class AttributedOpWithOperands(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.attributed_op_with_operands"
 // CHECK-NOT: _ODS_OPERAND_SEGMENTS
 // CHECK-NOT: _ODS_RESULT_SEGMENTS
 def AttributedOpWithOperands : TestOp<"attributed_op_with_operands"> {
-  // CHECK: def __init__(self, _gen_arg_0, in_, _gen_arg_2, is_, loc=None, ip=None):
+  // CHECK: def __init__(self, _gen_arg_0, in_, _gen_arg_2, is_, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   operands.append(_gen_arg_0)
   // CHECK:   operands.append(_gen_arg_2)
   // CHECK:   if bool(in_): attributes["in"] = _ods_ir.UnitAttr.get(
   // CHECK:     _ods_get_default_loc_context(loc))
   // CHECK:   if is_ is not None: attributes["is"] = is_
def AttributedOpWithOperands : TestOp<"attributed_op_with_operands"> {
   let arguments = (ins I32, UnitAttr:$in, F32, OptionalAttr<F32Attr>:$is);
 }
 
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class EmptyOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.empty"
 def EmptyOp : TestOp<"empty">;
-  // CHECK: def __init__(self, loc=None, ip=None):
+  // CHECK: def __init__(self, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class MissingNamesOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.missing_names"
 def MissingNamesOp : TestOp<"missing_names"> {
-  // CHECK: def __init__(self, i32, _gen_res_1, i64, _gen_arg_0, f32, _gen_arg_2, loc=None, ip=None):
+  // CHECK: def __init__(self, _gen_arg_0, f32, _gen_arg_2, i32, _gen_res_1, i64, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
-  // CHECK:   results.append(i32)
-  // CHECK:   results.append(_gen_res_1)
-  // CHECK:   results.append(i64)
   // CHECK:   operands.append(_gen_arg_0)
   // CHECK:   operands.append(f32)
   // CHECK:   operands.append(_gen_arg_2)
+  // CHECK:   results.append(i32)
+  // CHECK:   results.append(_gen_res_1)
+  // CHECK:   results.append(i64)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))
 
   // CHECK: @property
   // CHECK: def f32(self):
   // CHECK:   return self.operation.operands[1]
   let arguments = (ins I32, F32:$f32, I64);

 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class OneVariadicOperandOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.one_variadic_operand"
 // CHECK-NOT: _ODS_OPERAND_SEGMENTS
 // CHECK-NOT: _ODS_RESULT_SEGMENTS
 def OneVariadicOperandOp : TestOp<"one_variadic_operand"> {
-  // CHECK: def __init__(self, non_variadic, variadic, loc=None, ip=None):
+  // CHECK: def __init__(self, non_variadic, variadic, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   operands.append(non_variadic)
   // CHECK:   operands.extend(variadic)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))

 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class OneVariadicResultOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.one_variadic_result"
 // CHECK-NOT: _ODS_OPERAND_SEGMENTS
 // CHECK-NOT: _ODS_RESULT_SEGMENTS
 def OneVariadicResultOp : TestOp<"one_variadic_result"> {
-  // CHECK: def __init__(self, variadic, non_variadic, loc=None, ip=None):
+  // CHECK: def __init__(self, variadic, non_variadic, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   results.extend(variadic)
   // CHECK:   results.append(non_variadic)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))
def OneVariadicResultOp : TestOp<"one_variadic_result"> {
   // CHECK:   return self.operation.results[1 + _ods_variadic_group_length - 1]
   let results = (outs Variadic<AnyType>:$variadic, AnyType:$non_variadic);
 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class PythonKeywordOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.python_keyword"
 def PythonKeywordOp : TestOp<"python_keyword"> {
-  // CHECK: def __init__(self, in_, loc=None, ip=None):
+  // CHECK: def __init__(self, in_, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   operands.append(in_)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))
 
def SameVariadicResultSizeOp : TestOp<"same_variadic_result",
   let results = (outs Variadic<AnyType>:$variadic1, AnyType:$non_variadic,
                  Variadic<AnyType>:$variadic2);
 }
 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class SimpleOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.simple"
 def SimpleOp : TestOp<"simple"> {
-  // CHECK: def __init__(self, i64, f64, i32, f32, loc=None, ip=None):
+  // CHECK: def __init__(self, i32, f32, i64, f64, *, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
-  // CHECK:   results.append(i64)
-  // CHECK:   results.append(f64)
   // CHECK:   operands.append(i32)
   // CHECK:   operands.append(f32)
+  // CHECK:   results.append(i64)
+  // CHECK:   results.append(f64)
   // CHECK:   super().__init__(self._ods_build_default(
   // CHECK:     attributes=attributes, operands=operands, results=results,
   // CHECK:     loc=loc, ip=ip))
 
   // CHECK: @property
   // CHECK: def i32(self):
   // CHECK:   return self.operation.operands[0]
   //
   // CHECK: @property
   // CHECK: def f32(self):
   // CHECK:   return self.operation.operands[1]
   let arguments = (ins I32:$i32, F32:$f32);
 
   // CHECK: @property
   // CHECK: def i64(self):
   // CHECK:   return self.operation.results[0]
   //
   // CHECK: @property
   // CHECK: def f64(self):
   // CHECK:   return self.operation.results[1]
   let results = (outs I64:$i64, F64:$f64);
 }
+
+// CHECK: @_ods_cext.register_operation(_Dialect)
+// CHECK: class UnnamedUnaryResultOp(_ods_ir.OpView):
+// CHECK-LABEL: OPERATION_NAME = "test.unnamed_unary_result"
+def UnnamedUnaryResultOp : TestOp<"unnamed_unary_result"> {
+  // CHECK: def __init__(self, result, *, loc=None, ip=None):
+  let results = (outs I32);
+}

mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp

static bool isPythonKeyword(StringRef str) {
   return keywords.contains(str);
 }
 
 /// Checks whether `str` would shadow a generated variable or attribute
 /// part of the OpView API.
 static bool isODSReserved(StringRef str) {
   static llvm::StringSet<> reserved(
       {"attributes", "create", "context", "ip", "operands", "print", "get_asm",
-       "loc", "verify", "regions", "result", "results", "self", "operation",
+       "loc", "verify", "regions", "results", "self", "operation",
        "DIALECT_NAMESPACE", "OPERATION_NAME"});
   return str.startswith("_ods_") || str.endswith("_ods") ||
          reserved.contains(str);
 }
 
 /// Modifies the `name` in a way that it becomes suitable for Python bindings
 /// (does not change the `name` if it already is suitable) and returns the
 /// modified version.

 /// Populates `builderArgs` with the Python-compatible names of builder function
 /// arguments, first the results, then the intermixed attributes and operands in
 /// the same order as they appear in the `arguments` field of the op definition.
 /// Additionally, `operandNames` is populated with names of operands in their
 /// order of appearance.
 static void
 populateBuilderArgs(const Operator &op,
                     llvm::SmallVectorImpl<std::string> &builderArgs,
-                    llvm::SmallVectorImpl<std::string> &operandNames) {
-  for (int i = 0, e = op.getNumResults(); i < e; ++i) {
-    std::string name = op.getResultName(i).str();
-    if (name.empty())
-      name = llvm::formatv("_gen_res_{0}", i);
-    name = sanitizeName(name);
-    builderArgs.push_back(name);
-  }
+                    llvm::SmallVectorImpl<std::string> &operandNames,
+                    llvm::SmallVectorImpl<std::string> &resultNames) {
   for (int i = 0, e = op.getNumArgs(); i < e; ++i) {
     std::string name = op.getArgName(i).str();
     if (name.empty())
       name = llvm::formatv("_gen_arg_{0}", i);
     name = sanitizeName(name);
     builderArgs.push_back(name);
     if (!op.getArg(i).is<NamedAttribute *>())
       operandNames.push_back(name);
   }
+  for (int i = 0, e = op.getNumResults(); i < e; ++i) {
+    std::string name = op.getResultName(i).str();
+    if (name.empty()) {
+      if (op.getNumResults() == 1) {
+        // Special case for one result, make the default name be 'result'
+        // to properly match the built-in result accessor.
+        name = "result";
+      } else {
+        name = llvm::formatv("_gen_res_{0}", i);
+      }
+    }
+    name = sanitizeName(name);
+    builderArgs.push_back(name);
+    resultNames.push_back(name);
+  }
 }
 
 /// Populates `builderLines` with additional lines that are required in the
 /// builder to set up operation attributes. `argNames` is expected to contain
 /// the names of builder arguments that correspond to op arguments, i.e. to the
 /// operands and attributes in the same order as they appear in the `arguments`
 /// field.
 static void

 static void emitDefaultOpBuilder(const Operator &op, raw_ostream &os) {
   // If we are asked to skip default builders, comply.
   if (op.skipDefaultBuilders())
     return;
 
   llvm::SmallVector<std::string, 8> builderArgs;
   llvm::SmallVector<std::string, 8> builderLines;
   llvm::SmallVector<std::string, 4> operandArgNames;
+  llvm::SmallVector<std::string, 4> resultNames;
   builderArgs.reserve(op.getNumOperands() + op.getNumResults() +
                       op.getNumNativeAttributes());
-  populateBuilderArgs(op, builderArgs, operandArgNames);
-  populateBuilderLines(
-      op, "result",
-      llvm::makeArrayRef(builderArgs).take_front(op.getNumResults()),
-      builderLines, getNumResults, getResult);
+  populateBuilderArgs(op, builderArgs, operandArgNames, resultNames);
   populateBuilderLines(op, "operand", operandArgNames, builderLines,
                        getNumOperands, getOperand);
+  populateBuilderLines(op, "result", resultNames, builderLines, getNumResults,
+                       getResult);
   populateBuilderLinesAttr(
-      op, llvm::makeArrayRef(builderArgs).drop_front(op.getNumResults()),
+      op, llvm::makeArrayRef(builderArgs).drop_back(resultNames.size()),
       builderLines);
 
+  builderArgs.push_back("*");
   builderArgs.push_back("loc=None");
   builderArgs.push_back("ip=None");
   os << llvm::formatv(initTemplate, llvm::join(builderArgs, ", "),
                       llvm::join(builderLines, "\n    "));
 }
 
 static void constructAttributeMapping(const llvm::RecordKeeper &records,
                                       AttributeClasses &attributeClasses) {