[Mlir-commits] [mlir] [mlir-c] Add TypeConverter source and target materialization (PR #206528)

[Maksim Levental]

makslevental wrote:

## Adversarial review: `[mlir-c] Add TypeConverter source and target materialization`

Reviewed `mlir/include/mlir-c/Rewrite.h`, `mlir/lib/CAPI/Transforms/Rewrite.cpp`, and `mlir/test/CAPI/rewrite.c` against the wrapped C++ API in `mlir/include/mlir/Transforms/DialectConversion.h` and the conversion driver in `mlir/lib/Transforms/Utils/DialectConversion.cpp`. Findings grouped by the three focus areas.

### 1. C/C++ API concordance

**[major] Target materialization silently drops the `originalType` parameter.**
`Rewrite.cpp` (`wrapMaterializationCallback`) wraps both source and target callbacks into the unified C++ form `Value(OpBuilder&, Type, ValueRange, Location)` (4 args). For `addTargetMaterialization`, that selects the 4-arg overload `wrapTargetMaterialization` (`DialectConversion.h:523`), which discards the 5th argument `Type originalType`. The C++ docs (`DialectConversion.h:209-218`) are explicit that `originalType` "cannot be recovered from just outputType and inputs; that's why the originalType parameter exists." So C API consumers can *never* see the original type during a target materialization — a real capability of the wrapped API is unreachable through these bindings. At minimum this should be documented in the header; ideally the target callback signature should carry an `MlirType originalType` argument (the source/target callbacks then can't share one typedef, which is fine and arguably clearer). `Rewrite.h:611-619`, `Rewrite.cpp` `wrapMaterializationCallback`.

**[minor] 1:N (TypeRange / SmallVector<Value>) materializations are not exposed.** Both `add{Source,Target}Materialization` in C++ support `T = TypeRange` returning `SmallVector<Value>` (`DialectConversion.h:190-192, 220-221, 494-497`). The C callback takes a single `MlirType outputType` and returns a single `MlirValue`, so only 1:1 materializations are expressible. Source materialization is inherently 1:1 so this is only a gap for target. Acceptable as a first cut, but worth a one-line header note that 1:N is unsupported so users aren't surprised.

**[ok] `wrap(static_cast<RewriterBase*>(&builder))` is safe.** The driver invokes both materializations with a `RewriterBase &` (`DialectConversion.cpp:3352, 3365-3372`), and `RewriterBase : public OpBuilder` (`PatternMatch.h:368`), so the downcast is sound for the conversion-driver code path that actually calls these. The inline comment correctly scopes the claim to the conversion driver. Good.

**[ok] Null / ownership / return conventions.** `mlirValueIsNull(result) ? Value() : unwrap(result)` faithfully maps the C++ "return null to allow another materialization" contract (`DialectConversion.h:184-186`). The `assert(callback && ...)` matches the sibling legality/conversion bindings in the same file. `MlirValue *inputs` non-const matches the existing `MlirValue *operands` convention. Naming `MlirTypeConverterMaterializationCallback` is consistent with `MlirTypeConverterConversionCallback`. Header layering is unchanged. No ownership issues (the wrapped lambdas capture `callback`/`userData` by value, both trivially copyable). 

### 2. Test validity

**[ok] Both tests genuinely trigger materialization and are not vacuous.** The `assert(materializationCounter > 0 ...)` guard (incremented inside `buildCastMaterialization`) proves the registered callback actually ran, so neither test can pass while silently no-op'ing the new entry point. Source test: replacing `test.source` (i32) with `test.source_i64` (i64) leaves the legal `test.user` wanting i32, forcing a source materialization back to i32 — CHECK `test.cast (i64) -> i32` is correct. Target test: `test.consumer`'s i32 operand from the unconverted `test.producer` must become i64, forcing a target materialization — CHECK `test.cast (i32) -> i64` is correct. The two directions are real and distinct.

**[ok] CHECK anchoring / stream ordering.** `Operation::dump()` writes to `llvm::errs()` (stderr) and the labels use `fprintf(stderr, ...)`, and the RUN line is `2>&1 | FileCheck`. Since everything is on a single stream, interleaving is deterministic and the CHECK ordering is sound. This matches the established pattern in the rest of the file (e.g. `testReplaceOp`).

### 3. Test coverage gaps

**[minor] No coverage for the documented capability gaps.** Because `originalType` is dropped and 1:N is unsupported, there is correspondingly no test exercising a target materialization that inspects the original type, nor any multi-input/1:N materialization. If the binding is later extended (e.g. to pass `originalType`), there's no regression test pinning current behavior.

**[minor] The failure / return-null path is untested.** The wrapper's `mlirValueIsNull(result) ? Value() : ...` branch (the "this materialization declined, try the next" contract) is never exercised — both test callbacks always return a valid value. A second registered materialization where the first returns null would lock in this behavior.

**[nit] `nInputs > 1` is never exercised.** Both tests drive single-input materializations, so the `nInputs`/`inputs` marshalling loop is only covered for size 1. The non-1 case is what makes the count/pointer marshalling meaningful.

### Summary
Implementation is mechanically sound and the tests are valid (non-vacuous, correctly anchored). The one finding worth acting on before this lands is the **dropped `originalType`** for target materialization (major) — it makes a documented C++ capability unreachable from C with no note in the header. The 1:N and return-null/multi-input items are minor coverage/documentation gaps.


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