Hello, MLIR community!
I’d like to discuss with you some observations and ideas on writing C++ code in a MLIR-based compiler to create operations. I’m looking forward to learn from your feedback and even maybe discuss the proposal if deemed useful.
I do have some noticeable amount of code below, hope that’s not a problem as I wanted to get feedback before even getting close to build a patch for review. Suggestions for better format of discussion are welcome.
Note: code presented below does not assume using specific code-style or naming
Problem statement
Inconsistency
To illustrate the problem let me go through a few examples:
Let’s start with simple op defined in tablegen:
def TestOp : SomeDialect_Op<"test_op"> {
let arguments = (ins
I32:$requiredOperand0,
Optional<I32>:$optionalOperand1
);
let results = (outs I32);
}
Here we have 2 operands, first requiredOperand0
, then optionalOperand1
. We would be able to build an instance of this op using following C++ code:
builder.create<TestOp>(
location,
resultType,
someValue
);
So far, everything is nice, we were able to provide single mlir::Value
(someValue
) that would become our requiredOperand0
.
Looking at generated static member-functions for this TestBuilderOp
we see:
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, ::mlir::Value requiredOperand0, /*optional*/::mlir::Value optionalOperand1);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value requiredOperand0, /*optional*/::mlir::Value optionalOperand1);
static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
What was surprising to me is optionalOperand1
did not get default value generated in any of the signatures above. Our C++ builder.create
call worked because the last overload was picked with implicit conversion from mlir::Value
to mlir::ValueRange
.
Another even more surprising behavior to me was that this would work even with following tablegen code:
def TestOp : SomeDialect_Op<"test_op"> {
let arguments = (ins
Optional<I32>:$optionalOperand1,
I32:$requiredOperand0
);
let results = (outs I32);
}
We swapped operands and now optionalOperand1
is the first one, which leads to following build
static member-functions generated:
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, /*optional*/::mlir::Value optionalOperand1, ::mlir::Value requiredOperand0);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, /*optional*/::mlir::Value optionalOperand1, ::mlir::Value requiredOperand0);
static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
The reason it still works and the same builder.create
call assigns mlir::Value
to requiredOperand0
, not optionalOperand1
is implementation of getODSOperandIndexAndLength
that accounts for mix of variadic and non-varadic operands.
However, there’s some inconsistency IMO, because changing tablegen code doesn’t impact the behavior of builder.create
call above, but impacts the behavior of case when both mlir::Value
are provided:
builder.create<TestOp>(
location,
resultType,
someValue1,
someValue2
);
In this case with the last tablegen sample where optionalOperand1
is specified before requiredOperand0
, someValue1
is assigned to optionalOperand1
and someValue2
is assigned to requiredOperand0
. With the first tablegen sample the behavior is different and someValue1
is assigned to requiredOperand0
instead.
Another tricky moment is picking generic build
overload for builder.create
calls that try set only required operands does not always work.
If we have more than one requried operand:
def TestOp : SomeDialect_Op<"test_op"> {
let arguments = (ins
I32:$requiredOperand0,
I32:$requiredOperand1,
Optional<I32>:$optionalOperand2
);
let results = (outs I32);
}
We get following build
overloads:
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2);
static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
And now just passing 2 mlir::Value
objects to set 2 required operands won’t work anymore:
builder.create<TestOp>(
location,
resultType,
someValue1,
someValue2
);
We cannot pick first 2 overloads of build
because they require more operands (all of them require optionalOperand2
) and the last one (generic) cannot be picked because only someValue1
would be implicitly converted mlir::ValueRange
, the someValue2
would try to convert to argument for attributes and fail.
In order to make builder.create
call above to work we need explicitly pass mlir::ValueRange
:
builder.create<TestOp>(
location,
resultType,
mlir::ValueRange{someValue1, someValue2}
);
While default value for corresponding argument of
build
static member-function is not generated forOptional
orOptionalAttr
, it is generated forUnitAttr
andDefaultValuedAttr
.
Error handling
Other things that I would like to see improved are related to error handling. First scenario is the case when incorrect amount of arguments were passed during op creation. E.g. for the last case, if we do:
builder.create<TestOp>(
location,
resultType,
someValue1
);
// or
builder.create<TestOp>(
location,
resultType,
mlir::ValueRange{someValue1}
);
That would trigger an error, since there’s no mlir::Value
provided for requiredOperand1
, however the error is caught at run-time:
Ops.cpp.inc:1637: static void mlir::xxx::TestOp::build(::mlir::OpBuilder &, ::mlir::OperationState &, ::mlir::TypeRange, ::mlir::ValueRange, ::llvm::ArrayRef< ::mlir::NamedAttribute>): Assertion `operands.size() >= 2u && "mismatched number of parameters"' failed.
Second error scenario is when we do get compile-time error, but the error message is somewhat indirect. Consider this:
def TestOp : SomeDialect_Op<"test_op"> {
let arguments = (ins
I32:$requiredOperand0,
I32:$requiredOperand1,
I32:$requiredOperand2
);
let results = (outs I32);
}
// code generated for this
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, ::mlir::Value requiredOperand2);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, ::mlir::Value requiredOperand2);
static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
When we will try to compile
builder.create<TestOp>(
location,
resultType,
someValue1.getResult(),
someValue2.getResult()
);
We will get following compiler error:
llvm-project/mlir/include/mlir/IR/Builders.h:464:5: error: no matching function for call to 'build'
464 | OpTy::build(*this, state, std::forward<Args>(args)...);
| ^~~~~~~~~~~
note: in instantiation of function template specialization 'mlir::OpBuilder::create<mlir::xxx::TestOp, mlir::IntegerType &, mlir::Value, mlir::Value>' requested here
205 | auto testOp = builder.create<TestOp>(
| ^
note: candidate function not viable: no known conversion from 'mlir::Value' to '::llvm::ArrayRef< ::mlir::NamedAttribute>' for 5th argument
1023 | static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: candidate function not viable: requires 6 arguments, but 5 were provided
1021 | static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, ::mlir::Value requiredOperand2);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: candidate function not viable: requires 6 arguments, but 5 were provided
1022 | static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::Value requiredOperand0, ::mlir::Value requiredOperand1, ::mlir::Value requiredOperand2);
It does tell us what is wrong (amount of passed arguments), but error message is arguably not easy to read (it does get worse with more build
overloads provided by the user). Also, it cannot tell us which operand is missing exactly.
More realistic example inspired by a real codebase
Some modified sample of code in real project that motivated me to write this post:
builder.create<OpType>(
builder.getUnknownLoc(),
indexAttr,
/*taskLocation*/ nullptr,
barrierConfigConstOp.getOutput(),
bufferOp.getBuffer(),
previousTask,
waitBarriers,
updateBarriers,
/*startAfter*/ 0,
/*cleanAfter*/ 0,
false,
false,
false,
0,
DMAAccMode::DISABLE,
nullptr,
nullptr,
/*transactionAttr*/ nullptr,
dmaDescriptorAttr,
nullptr,
nullptr,
false,
nullptr,
enqueueBarrier,
wlmPageAttr
);
Clearly, there’s a design problem of OpType
having too many arguments, but I want to discuss different topic. This sample is used as just an illustration that situation can actually get extreme.
Note: even this specific example can get worse if more arguments have default value, like
0
,nullptr
orfalse
. MLIR generates multiplebuild
overloads that different in accepting either underlying type likebool
ormlir::Attribute
. So even if developer got arguments count right, there’s still a problem of getting to the right overload by e.g. usingfalse
instead ofnullptr
forUnitAttr
(or vice versa) for different arguments.
So, to conclude so far following problems were observed:
-
Difficulties to set only required operands without writing any code for missing optional operands
-
Not very user-friendly compile-time error messages or run-time errors instead of compile-time ones in case of incorrect builder call.
-
Inconsistency in behavior regarding how list of arguments to
builder.create
map into op definition in ODS -
Difficulties with picking the right
build
overload due to different attributes using underlying type ormlir::Attribute
Most if not all of it applies to attributes as well. Personally, I bothered by items 1 and 2 above the most.
Builder pattern as alternative solution
One idea I have been exploring to solve or at least improve problems above is inspired by (IMO quite interesting) talk by Ben Dean on cppcon 2018 Easy to Use, Hard to Misuse: Declarative Style in C++.
Basically, the idea is to write Builder pattern, but one that leverages strong types to guarantee at compile-time all required arguments were provided and generate more user-friendly error message if not.
Note: this idea is WIP and code samples below were tested in a limited scope, written manually and not auto-generated by tblgen, but hopefully is enough to kick-off the discussion.
To see how it could work, let’s take a look at this case:
def TestOp : SomeDialect_Op<"test_op", [SameVariadicOperandSize]> {
let arguments = (ins
Optional<I32>:$optionalOperand0,
I32:$requiredOperand1,
Optional<I32>:$optionalOperand2,
I32:$requiredOperand3,
OptionalAttr<I32Attr>:$optionalAttribute4
I32Attr:$requiredAttribute5
);
let results = (outs I32);
}
// build member-functions generated
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, /*optional*/::mlir::Value optionalOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2, ::mlir::Value requiredOperand3, /*optional*/::mlir::IntegerAttr optionalAttribute4, ::mlir::IntegerAttr requiredAttribute5);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, /*optional*/::mlir::Value optionalOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2, ::mlir::Value requiredOperand3, /*optional*/::mlir::IntegerAttr optionalAttribute4, ::mlir::IntegerAttr requiredAttribute5);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::Type resultType0, /*optional*/::mlir::Value optionalOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2, ::mlir::Value requiredOperand3, /*optional*/::mlir::IntegerAttr optionalAttribute4, uint32_t requiredAttribute5);
static void build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, /*optional*/::mlir::Value optionalOperand0, ::mlir::Value requiredOperand1, /*optional*/::mlir::Value optionalOperand2, ::mlir::Value requiredOperand3, /*optional*/::mlir::IntegerAttr optionalAttribute4, uint32_t requiredAttribute5);
static void build(::mlir::OpBuilder &, ::mlir::OperationState &odsState, ::mlir::TypeRange resultTypes, ::mlir::ValueRange operands, ::llvm::ArrayRef<::mlir::NamedAttribute> attributes = {});
Then, following code could be auto-generated for each operation:
// Ops.h.inc
template <uint64_t bits>
struct TestOpBuilderHelper;
template <>
struct TestOpBuilderHelper<0> {
// only required arguments / results need to be tracked
static constexpr auto RESULT_TYPE_BIT = size_t{1 << 0};
static constexpr auto REQUIRED_OPERAND_1_BIT = size_t{1 << 1};
static constexpr auto REQUIRED_OPERAND_3_BIT = size_t{1 << 2};
static constexpr auto REQUIRED_ATTRIBUTE_5_BIT = size_t{1 << 3};
static constexpr auto ALL_BITS = RESULT_TYPE_BIT | REQUIRED_OPERAND_1_BIT |
REQUIRED_OPERAND_3_BIT |
REQUIRED_ATTRIBUTE_5_BIT;
::mlir::Type resultType;
::mlir::Value optionalOperand0;
::mlir::Value requiredOperand1;
::mlir::Value optionalOperand2;
::mlir::Value requiredOperand3;
::mlir::IntegerAttr optionalAttribute4;
::mlir::IntegerAttr requiredAttribute5;
::mlir::xxx::TestOp build(::mlir::Location location,
::mlir::OpBuilder &odsBuilder);
};
using TestOpBuilder = TestOpBuilderHelper<TestOpBuilderHelper<0>::ALL_BITS>;
template <uint64_t N>
struct TestOpBuilderHelper : TestOpBuilderHelper<N - 1> {
using Base = TestOpBuilderHelper<0>;
// required arguments clear corresponding bit
TestOpBuilderHelper<N & ~Base::RESULT_TYPE_BIT>
setResultType(::mlir::Type resultType) {
this->resultType = resultType;
return *this;
}
// optional arguments return itself as-is
auto setOptionalOperand0(::mlir::Value value) {
this->optionalOperand0 = value;
return *this;
}
TestOpBuilderHelper<N & ~Base::REQUIRED_OPERAND_1_BIT>
setRequiredOperand1(::mlir::Value value) {
this->requiredOperand1 = value;
return *this;
}
auto setOptionalOperand2(::mlir::Value value) {
this->optionalOperand2 = value;
return *this;
}
TestOpBuilderHelper<N & ~Base::REQUIRED_OPERAND_3_BIT>
setRequiredOperand3(::mlir::Value value) {
this->requiredOperand3 = value;
return *this;
}
auto setOptionalAttribute4(::mlir::IntegerAttr value) {
this->optionalAttribute4 = value;
return *this;
}
TestOpBuilderHelper<N & ~Base::REQUIRED_ATTRIBUTE_5_BIT>
setRequiredAttribute5(::mlir::IntegerAttr value) {
this->requiredAttribute5 = value;
return *this;
}
::mlir::xxx::TestOp build(::mlir::Location, ::mlir::OpBuilder &) {
if constexpr (N & Base::REQUIRED_OPERAND_1_BIT) {
static_assert(false,
"an attempt to build TestOp with no requiredOperand1 set");
} else if constexpr (N & Base::REQUIRED_OPERAND_3_BIT) {
static_assert(false,
"an attempt to build TestOp with no requiredOperand3 set");
} else if constexpr (N & Base::REQUIRED_ATTRIBUTE_5_BIT) {
static_assert(
false, "an attempt to build TestOp with no requiredAttribute5 set");
} else {
static_assert(false, "unexpected error");
}
}
};
// Ops.cpp.inc
::mlir::xxx::TestOp
TestOpBuilderHelper<0>::build(::mlir::Location location,
::mlir::OpBuilder &odsBuilder) {
return odsBuilder.create<::mlir::xxx::TestOp>(
location, resultType, optionalOperand0, requiredOperand1,
optionalOperand2, requiredOperand3, optionalAttribute4,
requiredAttribute5);
}
With this, C++ code to build TestOp
would be:
TestOpBuilder{}
.setResultType(resultType)
.setRequiredOperand1(someValue1)
.setRequiredOperand3(someValue2)
.setRequiredAttribute5(someAttribute)
.build(location, builder);
It has some advantages: self-documenting code due to member-functions names, consistent behavior in terms what operand is assigned, no code for optional arguments, no dependency on order of calls.
Regarding error-handling, if we, for example, would make a mistake and forget to set some required operand:
TestOpBuilder{}
.setResultType(resultType)
.setRequiredOperand1(someValue1)
// .setRequiredOperand3(someValue2)
.setRequiredAttribute5(someAttribute)
.build(location, builder);
It’ll be following compile-time error:
error: static assertion failed: an attempt to build TestOp with no requiredOperand3 set
114 | static_assert(false,
| ^~~~~
note: in instantiation of member function 'TestOpBuilderHelper<4>::build' requested here
221 | .build(location, builder);
Note: as you can see so far the idea is to have new patter as addition to current infrastructure, not replacement!
There are definitely some remaining opens:
-
How well that’s going to work with properties? Current expectation is properties are different in a way they may not be cheap to copy in generated setters, maybe using move-assignments/constructors would solve it.
-
What if something does not have default constructor / move operations? Default constructor maybe solvable through using
std::optional
, but not sure. Hopefully, lack of move operations is not a real issue (it will be just a copy?). -
This implementation have the limit of up to 64 required things to be set - is it a problem? I’d say since new pattern is not a replacement, but rather opt-in feature, that’s okay. Especially, that having more than 64 required arguments is too much anyway, IMO.
-
What about new builder interacting with existing related code,
OpBuilder
, rewriters or anything else? -
Is template instantiation or template code here in general is a problem?
That’s all I did so far in this direction and I would like to get some feedback before proceeding with further experiments as there could be some unexpected to me blockers for this proposal, existing solution to the problems or better ideas in general.