I think only the positive tests for Attribute conversions are really relevant. And since that's covered by tests in the original change, it seems fine to drop this patch.

thanks for following up on this!

Is there still a reason to implement __str__ for StringAttr then? It seems to just add inconsistency, as the implementation on every other attribute gives the asm string.

In particular, pybind11 uses these functions to automatically cast objects to the corresponding primitive types wherever they are required by pybound functions, e.g., arguments are converted to Python's int if the C++ signature requires a C++ int.

In D157934#4610971, @stellaraccident wrote:

• Address the TODO about refactoring TD_FILE and the wrappers quickly so as not to have the transient GEN_ENUM_BINDINGS_TD_FILE baked into downstreams (changed CMake arguments like that are hard to track down).
• Add a comment to GEN_ENUM_BINDINGS_TD_FILE indicating that downstreams should not use this as it will be removed shortly/without warning upon satisfying the TODO.

Seems generally fine to me, but I'd wait to see if anyone has opinions on the approach (using Attribute.parse) or enum name changes.

The generated bindings: multiple dialects can emit tablegen records for the same enum attributes

In D156061#4535845, @springerm wrote:

In D156061#4534070, @rkayaith wrote:

In D156061#4530658, @Hardcode84 wrote:

I think static sizes/offsets can be generated from ssa ones by existing folders/canonicalization, so there are really no difference.

those would have to be updated to only fold if the constant value is valid

I think it would be OK to generate invalid ops during folding. (This can happen anyway, even if we do more aggressive checking of SSA values, because the verifier cannot take into account any possible foldings/canonicalizations that could happen.) garbage in => garbage out

In D156061#4530658, @Hardcode84 wrote:

I think static sizes/offsets can be generated from ssa ones by existing folders/canonicalization, so there are really no difference.

those would have to be updated to only fold if the constant value is valid

makes sense to me

In D153144#4432401, @springerm wrote:

In D153144#4428652, @mehdi_amini wrote:

Can this be tested without using the transform dialect?

I added a unit test. This issue can only be triggered from C++. (I cannot be triggered from test/Pass/pipeline-invalid.mlir.)

Awesome, just some minor comments.

makes sense to me

Thanks for all the iteration on this! Mostly minor comments here, LGTM once they're addressed but I'd like @ftynse / others to take a look as well.

So the (only?) bot testing this had its config updated in https://reviews.llvm.org/D150245, but it got shut down before the changes were applied. I'm going to go ahead and land this assuming it'll be updated if/when it comes back.

friendly ping

reopening so I remember to land this again once the bot is updated

LGTM, can you also update the revision title

In D148605#4277956, @mehdi_amini wrote:

In D148605#4277871, @rkayaith wrote:

In D148605#4276618, @mehdi_amini wrote:

Why is this a better API than the assertion?

It seems more consistent with the other methods, e.g. getDimSize(dim) doesn't assert that the dim size is known.

That does not seem comparable to me: it getDimSize(dim) returns always a valid dimension, if not known it return kDynamic, which is a valid value for a "dimension".
I don't see how getNumElements() can return anything "valid" when shapes are dynamic.

The interpretation would be that kDynamic is a valid value for a "shape element count" (maybe this isn't useful though, and actually I would've preferred returning something like std::optional<int64_t> instead, I just suggested kDynamic initially since it seemed like there was precedent).

In D148605#4276618, @mehdi_amini wrote:

Why is this a better API than the assertion?