[Mlir-commits] [mlir] [mlir][tblgen] Error on unsubstituted $_self in op trait predicates (PR #227263)

[llvmlistbot at llvm.org]

LiRvs-Miria wrote:

Thanks — having read through the stack, this is a much better direction than what I was proposing. The layered design answers my two questions more cleanly than either of my options: checked formatting as the substrate; eager fatal wrappers at the generator boundaries, where a formatting failure means a bug in our generators; and consuming the error to fall back to inline verification for speculative uniquing. No opt-out flag, no silent non-fatal path.

I'm happy to close this PR in favor of the stack — #238150 already covers my case (I see the ERROR18 test in `op-error.td` there), and the `$_self` docs I had planned overlap with the ODS doc updates in #238152. If it's useful as a repro anchor until the stack lands, I can keep it open — your call.

A few observations from reading the diffs; I'll add these as review comments on the individual PRs as I go through them:

1. The trait-context snapshot in #238150 (`FmtContext traitCtx = verifyCtx;` + the ERROR21 test) covers a case my patch would have missed — I verified this against my patch before commenting: the ERROR21 repro (an inline attribute predicate and an op trait both using `$_self`) exits `mlir-tblgen` cleanly with my marker check in place. The inline attribute path formats through `ctx.withSelf()`, which mutates the shared verifier context — so by the time the trait loop runs, `$_self` substitutes to the attribute variable and my marker never fires. The generated verifier then silently contains `if (!(tblgen_value)) return emitOpError("failed to verify that unbound self");` — a condition unrelated to the trait's intent. Nice catch.

2. One suggestion from the mistake that motivated this PR: with "available named substitutions: $_ctxt, $_op, $a, $b", a user who wrote `$_self` out of constraint-context habit still has to figure out the fix themselves. Would you consider attaching a remediation note when the failed placeholder is `$_self` in trait context ("use `$_op` to refer to the operation, or pre-substitute with `!subst`")? The two suggestions are existing idioms, not inventions — OpBase.td's `AllMatchSameOperatorPred` and `TypesMatchWith` both pre-substitute `$_self` via `!subst` internally. Happy to draft it.

3. On fallback semantics: when `canUniqueAttrConstraint` returns false, the constraint falls back to inline verification. I checked #238152: the inline branch formats through `tgfmtOrFatal` against a local copy of the context (`FmtContext valueCtx(ctx); valueCtx.withSelf(varName);`), so a genuinely broken template still gets a fatal diagnostic at tablegen time — with both the definition and use-site locations — and the temporary `$_self` binding no longer leaks into the shared context. That last part fixes the mechanism behind the ERROR21 case at the root, not just at the trait loop.

4. The decimal positional change in #238148 ($010 now means 10, not 8) is a silent behavior change for out-of-tree dialects. If not already covered, it may deserve a sentence in the ODS formatting doc / release notes.

5. What's the intended timeline for removing the deprecated API? Asking from the out-of-tree side — I maintain an IEC 61499 dialect and will need to migrate its tblgen usage. Happy to help test.

I'll review the stack bottom-up and verify my original repro end-to-end against `tgfmt-4`.

Assisted-by: GLM

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