[Mlir-commits] [mlir] [tblgen] Use `emitError` for inferResultTypes failures (PR #165488)

[Stef Lindall]

bethebunny wrote:

> If you have example where that isn't we should fix it.

It's not in precisely the place I'm updating.

```c++
nb::class_<FooOp>(m, "FooOp")
  .def("create", nb::overload_cast<OpBuilder&, Location>(&FooOp::create))
```

> If you have a NamedAttrList with repeated keys, it will assert fail.

I mean for failures in inferring result types

> Yes, this is returning a Operation* effectively rather than FooOp view. You'll see no one checks if a C++ Op::create returned null or functions that take it as input checks. So it would result in silent corruption/error at distance vs today as everyone expects it doesn't get to that.

This doesn't sound like how I'm using MLIR today at all :) but I acknowledge this is a common pattern and I don't want to break folk's existing usages. In all our usages `emitError()` signals a failure out-of-band, and that seems to be a common pattern upstream also.

> You don't need to though, I'm saying you can do something like
>  ```
>  template <class T>
>  FailureOr<T> createFailable(...) { ... }
>  ```
>  and then do exactly the same as is done in Python, and this just forwards to the underlying generated builders, so nothing extra to write.

I think this has the usage pattern backwards. I'm binding the C++ op builder calls, whether they're generated default builders, or specified via interface / c++ class definition, or have custom implementations etc. I can do this just fine in the normal case, however any of these builders which transitively calls a generated op that can fatal error is poison and I suddenly can't bind against. This means I have to rewrite custom implementations of both the generated op builders _and_ any op builders which happen to depend on those.

https://github.com/llvm/llvm-project/pull/165488