The current signature of regionBuilder functions is
using RegionBuilderFn = llvm::function_ref<void(ImplicitLocOpBuilder &, Block &,
ArrayRef<NamedAttribute>)>;
This makes it impossible to update the caller if anything went wrong (e.g., #132740). I’m curious to know if there are any objections to modifying this signature.
Github issue tracking this: [mlir] LinalgOps::regionBuilder function should return ParseResult · Issue #136324 · llvm/llvm-project · GitHub
One option is to have the builder return LogicalResult.
None of the builder APIs in MLIR are meant to be failable right now, what are we expecting to fail here and how would the caller of a linalg op builder catch this and recover?
One option is to have the builder return LogicalResult.
Makes sense. Thanks.
None of the builder APIs in MLIR are meant to be failable right now, what are we expecting to fail here and how would the caller of a linalg op builder catch this and recover?
From the example in [mlir] Linalg MatmulOp::parse crashes · Issue #132740 · llvm/llvm-project · GitHub
func.func @return_inside_func_yield(%t: tensor<?xf16>, %f: vector<4xf16>)
-> (tensor<?xf16>, vector<4xf16>)
{
%0 = linalg.matmul ins(%t, %t : tensor<?xf16>, tensor<?xf16>)
outs(%f : vector<4xf16>) -> tensor<?xf16>
func.return %0, %f : tensor<?xf16>, vector<4xf16>
}
It crashes in #7 0x000055c0b5a4e8fd (anonymous namespace)::RegionBuilderHelper::buildBinaryFn(mlir::linalg::BinaryFn, mlir::Value, mlir::Value) LinalgOps.cpp:
That function bails out when both inputs are not of the same types. llvm-project/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp at main · llvm/llvm-project · GitHub
if (!allComplex && !allFloatingPoint && !allInteger)
llvm_unreachable("unsupported non numeric type");
Not sure if this case is meant to be handled. What if the IR itself is incorrect?
I think “allowing the builders to fail” may be the wrong move here, it’s a bit wide-ranging. Perhaps we need a builtin.invalid "reason" (args) : (argTys) -> (resultTys) op that can be used to signal the failure and we can just build that in the bad case. (And said op is defined to always fail verification.)
(I’d make it builtin, since I don’t think there’s an obvious dialect for it)
Custom assembly parser can be fragile with invalid IR.
Incorrect IR is by default printed with the generic form for this reason.
In general though it is the responsibility of the parser code (so parseNamedStructuredOp() I guess) to check the precondition of the builder they will invoke, diagnose, and fail accordingly.
Custom assembly parser can be fragile with invalid IR.
..
In general though it is the responsibility of the parser code (so parseNamedStructuredOp() I guess) to check the precondition of the builder they will invoke, diagnose, and fail accordingly.
This may lead to code duplication and unknown amount of look-ahead depending on the syntax. For some languages(dialects) this is suitable if they are simple enough.
If we don’t want to return error from builder, IMO, @ krzysz00’s idea of introducing a builtin.invalid "reason" (args) : (argTys) -> (resultTys) essentially an invalid terminal, seems more scalable.
It’s the other way around IMO: the syntax must be designed to avoid this kind of problem
In this case everything is parsed within parseNamedStructuredOp, and any kind of sanity checks could be implemented here.
Also here the linalg.matmul operation is entirely parsed before calling the building methods: there is no need for any look ahead.
I’m not convinced about the builtin.invalid “solution”: it seems more like a workaround/hack than anything.
The builtin.invalid won’t work with the builder API. A builder.create<FooOp> is expected to create and return an instance of FooOp, it cannot return an instance of another op class. Technically, ops are nullable and .create could return null on failure, but it will break an extremely widespread invariant that allows us not to check the result of .create for null.
We intentionally restricted the op parser API to penalize “inventiveness” in the syntax. It should remain simple and somewhat consistent, we are parsing an IR and not a language after all.
Regarding code duplication with verifiers, this is not a new problem. We can factor out the verification logic into a separate function taking ValueRange operands, TypeRange resultTypes etc. instead of Operation * and call this function from the verifier and the parser.
Agreed that extracting the logic from the verifier so it can be checked at parse-time is probably also the right call here