[mlir][tblgen][ods][python] Use keyword-only arguments for optional builder arguments in generated Python bindings

[jfurtek]

Summary

This diff modifies mlir-tblgen to generate Python Operation class __init__()
functions that use Python keyword-only arguments.

Previously, all __init__() function arguments were positional. Python code to
create MLIR Operations was required to provide values for ALL builder arguments,
including optional arguments (attributes and operands). Callers that did not
provide, for example, an optional attribute would be forced to provide None
as an argument for EACH optional attribute. Proposed changes in this diff use
tblgen record information (as provided by ODS) to generate keyword arguments
for:

• optional operands
• optional attributes (which includes unit attributes)
• default-valued attributes

These __init__() function keyword arguments have default None values (i.e.
the argument form is optionalAttr=None), allowing callers to create Operations
more easily.

Note that since optional arguments become keyword-only arguments (since they are
placed after the bare * argument), this diff will require ALL optional
operands and attributes to be provided using explicit keyword syntax. This may,
in the short term, break any out-of-tree Python code that provided values via
positional arguments. However, in the long term, it seems that requiring
keywords for optional arguments will be more robust to operation changes that
add arguments.

Tests were modified to reflect the updated Operation builder calling convention.

This diff partially addresses the requests made in the github issue below.

https://github.com/llvm/llvm-project/issues/54932

[jpienaar]

(I'm OOO, but randomly checking so my latency will be high)

I was thinking about this recently for a change I was playing around with (moving result types so that adding build time type inference doesn't change signature). For these I was wondering what if we follow what we do for C++ and we only add defaults for the "trailing" defaults. Both C++ and Python have the same restriction wrt default valued arguments and this then keeps the argument order as specified in ODS, used in C++, docs generated for dialect and DRR. Then the dialect author has the choice (and semi encouraged to do so, as today, except C++ allows overloading which lessens encouragement) to have default valued attributes towards end (we could even add log a message). Point of discussion, esp as I don't know Python conventions, but keeping these consistent is appealing to me.

I also realized that we'd hit another issue (unrelated to this change but to default attributes and Python): the default attributes are currently lazily constructed in the C++ API while Python side this would not trigger. So querying the attribute would not return the default value if unset.

[stellaraccident]

In D124717#3488459, @jpienaar wrote:

(I'm OOO, but randomly checking so my latency will be high)

I was thinking about this recently for a change I was playing around with (moving result types so that adding build time type inference doesn't change signature). For these I was wondering what if we follow what we do for C++ and we only add defaults for the "trailing" defaults. Both C++ and Python have the same restriction wrt default valued arguments and this then keeps the argument order as specified in ODS, used in C++, docs generated for dialect and DRR. Then the dialect author has the choice (and semi encouraged to do so, as today, except C++ allows overloading which lessens encouragement) to have default valued attributes towards end (we could even add log a message). Point of discussion, esp as I don't know Python conventions, but keeping these consistent is appealing to me.

I also realized that we'd hit another issue (unrelated to this change but to default attributes and Python): the default attributes are currently lazily constructed in the C++ API while Python side this would not trigger. So querying the attribute would not return the default value if unset.

Jacques: what is your preference here on proceeding? If we're going to be breaking things, I have a minor preference to do so all at once if not too bad (vs a sequence of incremental breaks to the same API)

[jpienaar]

I agree on breaking at once, too many breaking changes one after the other is painful (looking at JAX team needing to bump API number for small changes makes me feel bad). Perhaps we could roll https://gist.github.com/jpienaar/9d22b710db6122a9a41e8cd3d824a39e or something like it in here too (also breaking change but orthogonal to this one, but in similar spirit). Ripping off bandaid should result in one larger change but enable less breaking changes post. I do have preference for consistency with ODS and C++ here (it may also result in less breakages but that's not the reason why I prefer it). It puts convenience and usability in the hand of dialect authors. Now the signature can change if one changes the op def, but merely appending optional/default value attribute would not. But I might not be a "pythonic" person :-)

I can do follow up on removing lazy initialization once back as i think it doesn't pay for itself and makes things behave unexpected ways.

[mikeurbach]

Apologies for the delay... I said I'd take a look at this from CIRCT's perspective a week ago. Going this route in general sounds good to me, and the current implementation seems to make sense. I'm happy to update CIRCT as needed.

[jfurtek]

(rebase)

[mikeurbach]

I took this for a spin in CIRCT, and we have already used the extension mechanism to write our own constructors in a lot of places this could break APIs. There was one CIRCT helper, used in two places, that required minor changes at the call sites. I just wrote up a patch since it was so simple: https://github.com/llvm/circt/pull/3134.

So, +1 from me, the kinds of breakage this causes are generally pretty trivial, and at least for CIRCT, already fixed. It might be more annoying for other downstreams that rely on the generated constructors more directly, but as far as MLIR breakages go, they are not a big deal IMHO, especially since you've already used Discourse to raise visibility about this change potentially coming. But thanks for waiting for our feedback on the CIRCT side!

[jfurtek]

• Fix clang-format errors

[jfurtek]

• Update _ml_program_ops_ext.py to use FuncOp keyword arg

[jfurtek]

@stellaraccident - I don't have commit privileges. Can you (or someone else) land this diff at your convenience? Thanks!

[stellaraccident]

Yes, I will land it either tomorrow or this weekend. Thank you for the patch!

mlir/python/mlir/dialects/_func_ops_ext.py

def __init__(self,
 
     # If the type is passed as a tuple, build a FunctionType on the fly.
     if isinstance(type, tuple):
       type = FunctionType.get(inputs=type[0], results=type[1])
 
     type = TypeAttr.get(type)
     sym_visibility = StringAttr.get(
         str(visibility)) if visibility is not None else None
-    super().__init__(sym_name, type, sym_visibility, loc=loc, ip=ip)
+    super().__init__(sym_name, type, sym_visibility=sym_visibility, loc=loc, ip=ip)
     if body_builder:
       entry_block = self.add_entry_block()
       with InsertionPoint(entry_block):
         body_builder(self)
 
   @property
   def is_external(self):
     return len(self.regions[0].blocks) == 0

mlir/python/mlir/dialects/_ml_program_ops_ext.py

def __init__(self,
 
     # If the type is passed as a tuple, build a FunctionType on the fly.
     if isinstance(type, tuple):
       type = FunctionType.get(inputs=type[0], results=type[1])
 
     type = TypeAttr.get(type)
     sym_visibility = StringAttr.get(
         str(visibility)) if visibility is not None else None
-    super().__init__(sym_name, type, sym_visibility, loc=loc, ip=ip)
+    super().__init__(sym_name, type, sym_visibility=sym_visibility, loc=loc, ip=ip)
     if body_builder:
       entry_block = self.add_entry_block()
       with InsertionPoint(entry_block):
         body_builder(self)
 
   @property
   def is_external(self):
     return len(self.regions[0].blocks) == 0

mlir/python/mlir/dialects/_pdl_ops_ext.py

class AttributeOp:
   def __init__(self,
                type: Optional[Union[OpView, Operation, Value]] = None,
                value: Optional[Attribute] = None,
                *,
                loc=None,
                ip=None):
     type = type if type is None else _get_value(type)
     result = pdl.AttributeType.get()
-    super().__init__(result, type, value, loc=loc, ip=ip)
+    super().__init__(result, type=type, value=value, loc=loc, ip=ip)
 
 
 class EraseOp:
   """Specialization for PDL erase op class."""
 
   def __init__(self,
                operation: Optional[Union[OpView, Operation, Value]] = None,
                *,
                loc=None,
                ip=None):
     operation = _get_value(operation)
     super().__init__(operation, loc=loc, ip=ip)
 
 
 class OperandOp:
   """Specialization for PDL operand op class."""
 
   def __init__(self,
                type: Optional[Union[OpView, Operation, Value]] = None,
                *,
                loc=None,
                ip=None):
     type = type if type is None else _get_value(type)
     result = pdl.ValueType.get()
-    super().__init__(result, type, loc=loc, ip=ip)
+    super().__init__(result, type=type, loc=loc, ip=ip)
 
 
 class OperandsOp:
   """Specialization for PDL operands op class."""
 
   def __init__(self,
                types: Optional[Union[OpView, Operation, Value]] = None,
                *,
                loc=None,
                ip=None):
     types = types if types is None else _get_value(types)
     result = pdl.RangeType.get(pdl.ValueType.get())
-    super().__init__(result, types, loc=loc, ip=ip)
+    super().__init__(result, type=types, loc=loc, ip=ip)
 
 
 class OperationOp:
   """Specialization for PDL operand op class."""
 
   def __init__(self,
                name: Optional[Union[str, StringAttr]] = None,
                args: Sequence[Union[OpView, Operation, Value]] = [],
                attributes: Mapping[str, Union[OpView, Operation, Value]] = {},
                types: Sequence[Union[OpView, Operation, Value]] = [],
                *,
                loc=None,
                ip=None):
     name = name if name is None else _get_str_attr(name)
     args = _get_values(args)
     attributeNames = []
     attributeValues = []
     for attrName, attrValue in attributes.items():
       attributeNames.append(StringAttr.get(attrName))
       attributeValues.append(_get_value(attrValue))
     attributeNames = ArrayAttr.get(attributeNames)
     types = _get_values(types)
     result = pdl.OperationType.get()
-    super().__init__(result, name, args, attributeValues, attributeNames, types, loc=loc, ip=ip)
+    super().__init__(result, args, attributeValues, attributeNames, types, name=name, loc=loc, ip=ip)
 
 
 class PatternOp:
   """Specialization for PDL pattern op class."""
 
   def __init__(self,
                benefit: Union[IntegerAttr, int],
                name: Optional[Union[StringAttr, str]] = None,
                *,
                loc=None,
                ip=None):
     """Creates an PDL `pattern` operation."""
     name_attr = None if name is None else _get_str_attr(name)
     benefit_attr = _get_int_attr(16, benefit)
-    super().__init__(benefit_attr, name_attr, loc=loc, ip=ip)
+    super().__init__(benefit_attr, sym_name=name_attr, loc=loc, ip=ip)
     self.regions[0].blocks.append()
 
   @property
   def body(self):
     """Return the body (block) of the pattern."""
     return self.regions[0].blocks[0]
 
 
 class ReplaceOp:
   """Specialization for PDL replace op class."""
 
   def __init__(self,
                op: Union[OpView, Operation, Value],
                *,
                with_op: Optional[Union[OpView, Operation, Value]] = None,
                with_values: Sequence[Union[OpView, Operation, Value]] = [],
                loc=None,
                ip=None):
     op = _get_value(op)
     with_op = with_op if with_op is None else _get_value(with_op)
     with_values = _get_values(with_values)
-    super().__init__(op, with_op, with_values, loc=loc, ip=ip)
+    super().__init__(op, with_values, replOperation=with_op, loc=loc, ip=ip)
 
 
 class ResultOp:
   """Specialization for PDL result op class."""
 
   def __init__(self,
                parent: Union[OpView, Operation, Value],
                index: Union[IntegerAttr, int],
def __init__(self,
                result: Type,
                parent: Union[OpView, Operation, Value],
                index: Optional[Union[IntegerAttr, int]] = None,
                *,
                loc=None,
                ip=None):
     parent = _get_value(parent)
     index = index if index is None else _get_int_attr(32, index)
-    super().__init__(result, parent, index, loc=loc, ip=ip)
+    super().__init__(result, parent, index=index, loc=loc, ip=ip)
 
 
 class RewriteOp:
   """Specialization for PDL rewrite op class."""
 
   def __init__(self,
                root: Optional[Union[OpView, Operation, Value]] = None,
                name: Optional[Union[StringAttr, str]] = None,
                args: Sequence[Union[OpView, Operation, Value]] = [],
                *,
                loc=None,
                ip=None):
     root = root if root is None else _get_value(root)
     name = name if name is None else _get_str_attr(name)
     args = _get_values(args)
-    super().__init__(root, name, args, loc=loc, ip=ip)
+    super().__init__(args, root=root,name=name, loc=loc, ip=ip)
 
   def add_body(self):
     """Add body (block) to the rewrite."""
     self.regions[0].blocks.append()
     return self.body
 
   @property
   def body(self):
     """Return the body (block) of the rewrite."""
     return self.regions[0].blocks[0]
 
 
 class TypeOp:
   """Specialization for PDL type op class."""
 
   def __init__(self,
                type: Optional[Union[TypeAttr, Type]] = None,
                *,
                loc=None,
                ip=None):
     type = type if type is None else _get_type_attr(type)
     result = pdl.TypeType.get()
-    super().__init__(result, type, loc=loc, ip=ip)
+    super().__init__(result, type=type, loc=loc, ip=ip)
 
 
 class TypesOp:
   """Specialization for PDL types op class."""
 
   def __init__(self,
                types: Sequence[Union[TypeAttr, Type]] = [],
                *,
                loc=None,
                ip=None):
     types = _get_array_attr([_get_type_attr(ty) for ty in types])
     types = None if not types else types
     result = pdl.RangeType.get(pdl.TypeType.get())
-    super().__init__(result, types, loc=loc, ip=ip)
+    super().__init__(result, types=types, loc=loc, ip=ip)

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

class TestOp<string mnemonic, list<Trait> 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,0,]
 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=None, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   regions = None
   // CHECK:   operands.append(_get_op_results_or_values(variadic1))
   // CHECK:   operands.append(_get_op_result_or_value(non_variadic))
   // CHECK:   operands.append(_get_op_result_or_value(variadic2) if variadic2 is not None else None)
   // CHECK:   _ods_successors = None

 
 
 // 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, in_, *, optionalF32Attr=None, unitAttr=None, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   regions = None
   // 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: @_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, _gen_arg_2, *, in_=None, is_=None, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   regions = None
   // CHECK:   operands.append(_get_op_result_or_value(_gen_arg_0))
   // CHECK:   operands.append(_get_op_result_or_value(_gen_arg_2))
   // CHECK:   if bool(in_): attributes["in"] = _ods_ir.UnitAttr.get(
   // CHECK:     _ods_get_default_loc_context(loc))

 
 // CHECK: @_ods_cext.register_operation(_Dialect)
 // CHECK: class OneOptionalOperandOp(_ods_ir.OpView):
 // CHECK-LABEL: OPERATION_NAME = "test.one_optional_operand"
 // CHECK-NOT: _ODS_OPERAND_SEGMENTS
 // CHECK-NOT: _ODS_RESULT_SEGMENTS
 def OneOptionalOperandOp : TestOp<"one_optional_operand"> {
   let arguments = (ins AnyType:$non_optional, Optional<AnyType>:$optional);
-  // CHECK: def __init__(self, non_optional, optional, *, loc=None, ip=None):
+  // CHECK: def __init__(self, non_optional, *, optional=None, loc=None, ip=None):
   // CHECK:   operands = []
   // CHECK:   results = []
   // CHECK:   attributes = {}
   // CHECK:   regions = None
   // CHECK:   operands.append(_get_op_result_or_value(non_optional))
   // CHECK:   if optional is not None: operands.append(_get_op_result_or_value(optional))
   // CHECK:   _ods_successors = None
   // CHECK:   super().__init__(self.build_generic(

mlir/test/python/dialects/python_test.py

with Context() as ctx, Location.unknown():
     two = IntegerAttr.get(i32, 2)
     unit = UnitAttr.get()
 
     # CHECK: "python_test.attributed_op"() {
     # CHECK-DAG: mandatory_i32 = 1 : i32
     # CHECK-DAG: optional_i32 = 2 : i32
     # CHECK-DAG: unit
     # CHECK: }
-    op = test.AttributedOp(one, two, unit)
+    op = test.AttributedOp(one, optional_i32=two, unit=unit)
     print(f"{op}")
 
     # CHECK: "python_test.attributed_op"() {
     # CHECK: mandatory_i32 = 2 : i32
     # CHECK: }
-    op2 = test.AttributedOp(two, None, None)
+    op2 = test.AttributedOp(two)
     print(f"{op2}")
 
     #
     # Check generic "attributes" access and mutation.
     #
 
     assert "additional" not in op.attributes
 

 @run
 def testOptionalOperandOp():
   with Context() as ctx, Location.unknown():
     test.register_python_test_dialect(ctx)
 
     module = Module.create()
     with InsertionPoint(module.body):
 
-      op1 = test.OptionalOperandOp(None)
+      op1 = test.OptionalOperandOp()
       # CHECK: op1.input is None: True
       print(f"op1.input is None: {op1.input is None}")
 
-      op2 = test.OptionalOperandOp(op1)
+      op2 = test.OptionalOperandOp(input=op1)
       # CHECK: op2.input is None: False
       print(f"op2.input is None: {op2.input is None}")
 
 
 # CHECK-LABEL: TEST: testCustomAttribute
 @run
 def testCustomAttribute():
   with Context() as ctx:

mlir/test/python/dialects/vector.py

with InsertionPoint(module.body):
     identity_map = AffineMap.get_identity(vector_type.rank)
     identity_map_attr = AffineMapAttr.get(identity_map)
     f = func.FuncOp("transfer_read",
                           ([memref_type, index_type,
                             F32Type.get(), mask_type], []))
     with InsertionPoint(f.add_entry_block()):
       A, zero, padding, mask = f.arguments
       vector.TransferReadOp(vector_type, A, [zero, zero], identity_map_attr,
-                            padding, mask, None)
+                            padding, mask=mask)
       vector.TransferReadOp(vector_type, A, [zero, zero], identity_map_attr,
-                            padding, None, None)
+                            padding)
       func.ReturnOp([])
 
   # CHECK: @transfer_read(%[[MEM:.*]]: memref<?x?xf32>, %[[IDX:.*]]: index,
   # CHECK: %[[PAD:.*]]: f32, %[[MASK:.*]]: vector<2x3xi1>)
   # CHECK: vector.transfer_read %[[MEM]][%[[IDX]], %[[IDX]]], %[[PAD]], %[[MASK]]
   # CHECK: vector.transfer_read %[[MEM]][%[[IDX]], %[[IDX]]], %[[PAD]]
   # CHECK-NOT: %[[MASK]]
   print(module)

mlir/tools/mlir-tblgen/OpPythonBindingGen.cpp

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);
   }
+}
+
+/// Populates `builderArgs` with the Python-compatible names of builder function
+/// successor arguments. Additionally, `successorArgNames` is also populated.
+static void populateBuilderArgsSuccessors(
+    const Operator &op, llvm::SmallVectorImpl<std::string> &builderArgs,
+    llvm::SmallVectorImpl<std::string> &successorArgNames) {
 
   for (int i = 0, e = op.getNumSuccessors(); i < e; ++i) {
     NamedSuccessor successor = op.getSuccessor(i);
     std::string name = std::string(successor.name);
     if (name.empty())
       name = llvm::formatv("_gen_successor_{0}", i);
     name = sanitizeName(name);
     builderArgs.push_back(name);
static void emitDefaultOpBuilder(const Operator &op, raw_ostream &os) {
   llvm::SmallVector<std::string> builderLines;
   llvm::SmallVector<std::string> operandArgNames;
   llvm::SmallVector<std::string> successorArgNames;
   builderArgs.reserve(op.getNumOperands() + op.getNumResults() +
                       op.getNumNativeAttributes() + op.getNumSuccessors());
   populateBuilderArgsResults(op, builderArgs);
   size_t numResultArgs = builderArgs.size();
   populateBuilderArgs(op, builderArgs, operandArgNames, successorArgNames);
+  size_t numOperandAttrArgs = builderArgs.size() - numResultArgs;
+  populateBuilderArgsSuccessors(op, builderArgs, successorArgNames);
 
   populateBuilderLinesOperand(op, operandArgNames, builderLines);
   populateBuilderLinesAttr(
       op, llvm::makeArrayRef(builderArgs).drop_front(numResultArgs),
       builderLines);
   populateBuilderLinesResult(
       op, llvm::makeArrayRef(builderArgs).take_front(numResultArgs),
       builderLines);
   populateBuilderLinesSuccessors(op, successorArgNames, builderLines);
   populateBuilderRegions(op, builderArgs, builderLines);
 
-  builderArgs.push_back("*");
-  builderArgs.push_back("loc=None");
-  builderArgs.push_back("ip=None");
-  os << llvm::formatv(initTemplate, llvm::join(builderArgs, ", "),
+  // Layout of builderArgs vector elements:
+  // [ result_args  operand_attr_args successor_args regions ]
+
+  // Determine whether the argument corresponding to a given index into the
+  // builderArgs vector is a python keyword argument or not.
+  auto isKeywordArgFn = [&](size_t builderArgIndex) -> bool {
+    // All result, successor, and region arguments are positional arguments.
+    if ((builderArgIndex < numResultArgs) ||
+        (builderArgIndex >= (numResultArgs + numOperandAttrArgs)))
+      return false;
+    // Keyword arguments:
+    // - optional named attributes (including unit attributes)
+    // - default-valued named attributes
+    // - optional operands
+    Argument a = op.getArg(builderArgIndex - numResultArgs);
+    if (auto *nattr = a.dyn_cast<NamedAttribute *>())
+      return (nattr->attr.isOptional() || nattr->attr.hasDefaultValue());
+    else if (auto *ntype = a.dyn_cast<NamedTypeConstraint *>())
+      return ntype->isOptional();
+    else
+      return false;
+  };
+
+  // StringRefs in functionArgs refer to strings allocated by builderArgs.
+  llvm::SmallVector<llvm::StringRef> functionArgs;
+
+  // Add positional arguments.
+  for (size_t i = 0, cnt = builderArgs.size(); i < cnt; ++i) {
+    if (!isKeywordArgFn(i))
+      functionArgs.push_back(builderArgs[i]);
+  }
+
+  // Add a bare '*' to indicate that all following arguments must be keyword
+  // arguments.
+  functionArgs.push_back("*");
+
+  // Add a default 'None' value to each keyword arg string, and then add to the
+  // function args list.
+  for (size_t i = 0, cnt = builderArgs.size(); i < cnt; ++i) {
+    if (isKeywordArgFn(i)) {
+      builderArgs[i].append("=None");
+      functionArgs.push_back(builderArgs[i]);
+    }
+  }
+  functionArgs.push_back("loc=None");
+  functionArgs.push_back("ip=None");
+  os << llvm::formatv(initTemplate, llvm::join(functionArgs, ", "),
                       llvm::join(builderLines, "\n    "));
 }
 
 static void constructAttributeMapping(const llvm::RecordKeeper &records,
                                       AttributeClasses &attributeClasses) {
   for (const llvm::Record *rec :
        records.getAllDerivedDefinitions("PythonAttr")) {
     attributeClasses.try_emplace(rec->getValueAsString("cppStorageType").trim(),