[Mlir-commits] [mlir] [mlir-c] Add RewriterBase insertion point save/restore (PR #206531)

[Maksim Levental]

makslevental wrote:

## Adversarial review: insertion-point save/restore C API

Reviewed `mlir/include/mlir-c/Rewrite.h`, `mlir/lib/CAPI/Transforms/Rewrite.cpp`, and `mlir/test/CAPI/rewrite.c` against C++ `OpBuilder::InsertPoint` / `saveInsertionPoint` / `restoreInsertionPoint` in `mlir/include/mlir/IR/Builders.h`. Grouped by the three requested areas.

### 1. Test validity — mostly solid

The happy path is genuinely non-vacuous. The final `CHECK` block requires `op_restored` to land **between** `op1` and `op2`, while `op_end` lands last. If restore were a no-op (IP "stayed at end" from the prior `setInsertionPointToEnd`), `op_restored` would appear after `op2`/`op_end` and the ordered `CHECK-NEXT` chain would fail. So the test does distinguish "restored to saved position" from "stayed at end." The `CHECK-LABEL` / `CHECK-NEXT` anchoring is correct and ordered.

No blocker here, but see area 2 — the test does not exercise the two restore branches that are unique to this PR.

### 2. Test coverage — real gaps (major)

`mlirRewriterBaseRestoreInsertionPoint` has three branches:
1. null block -> `clearInsertionPoint()`
2. null `operationAfter` -> `setInsertionPointToEnd(block)`
3. otherwise -> `setInsertionPoint(block, Block::iterator(op))`

The test only ever calls restore once, with `saved` (a non-null op), so **only branch 3 is exercised**. Branches 1 and 2 — the two new code paths whose correctness is least obvious — are never invoked through `mlirRewriterBaseRestoreInsertionPoint`:

- **End-of-block round-trip is not actually verified** (`Rewrite.cpp` restore, null-op branch). The test saves `endIp`, asserts `operationAfter` is null, then abandons it and inserts via the still-current end IP. It never *restores* `endIp` and inserts to prove the null-op encoding re-establishes end-of-block. Suggest: after moving the IP elsewhere, `restoreInsertionPoint(rewriter, endIp)`, insert an op, and `CHECK` it lands at end.
- **Cleared round-trip is not actually verified** (restore null-block branch). The test clears via the standalone `mlirRewriterBaseClearInsertionPoint` and checks the *save* side returns a null block (`clearedIp`). It never feeds a null-block point back into `restoreInsertionPoint` to confirm restore-of-cleared clears the IP. Suggest: set a real IP, then `restoreInsertionPoint(rewriter, clearedIp)`, then assert `mlirRewriterBaseGetInsertionBlock` is null.

Other untested-but-arguably-out-of-scope cases: saving when no IP has ever been set (fresh rewriter -> is `saveInsertionPoint` guaranteed `!isSet()`?), and the mid-block `operationAfter` case where restore targets a *different* block than the current one. At minimum the two round-trips above should be closed since they are the raison d'etre of the PR.

### 3. C/C++ API concordance

- **`operationAfter` is a misnomer and contradicts its own doc comment (major).** In `Rewrite.h` the field is named `operationAfter`, but the comment immediately says it is "the operation that subsequent insertions go **before**." That matches C++: `InsertPoint::getPoint()` is a `Block::iterator` and `setInsertionPoint(op)` inserts *before* `op`. The name says "After," the semantics say "before." The test confirms the "before" semantics (`SetInsertionPointBefore(op2)` -> `saved.operationAfter == op2`). Recommend renaming to `operationBefore` (or just `operation`) so the field name stops fighting its documentation. Note this is also inconsistent in spirit with the existing `mlirRewriterBaseGetOperationAfterInsertion`, but that prior name is likewise debatable; introducing a clearly-documented `before` field now is the cleaner choice.
- **Null encoding vs `isSet()` — correct.** `!isSet()` (null C++ block) maps to `{ {nullptr}, {nullptr} }`, and restore keys off `mlirBlockIsNull` -> `clearInsertionPoint()`. Faithful.
- **`getPoint() == block->end()` handling — correct.** End-of-block maps to null `operationAfter`; restore reconstructs via `setInsertionPointToEnd(block)` == `setInsertionPoint(block, block->end())`. Round-trips faithfully (just unverified, area 2).
- **`Block::iterator(unwrap(...))` reconstruction — correct.** Matches C++ `setInsertionPoint(Operation*)` which builds `Block::iterator(op)`. The PR passes `block` explicitly rather than `op->getBlock()`; equivalent here since the saved block is the op's block.
- **Header layering — fine.** `MlirBlock`/`MlirOperation` come from already-included `mlir-c/IR.h`; no new include needed.
- **nit:** `wrap(&*ip.getPoint())` vs the neighboring `wrap(std::addressof(*it))` in `mlirRewriterBaseGetOperationAfterInsertion` — same effect, minor style inconsistency.

### Summary
- Blocker: none.
- Major: (a) `operationAfter` field name contradicts its doc/semantics; (b) the null-op (end-of-block) and null-block (cleared) **restore** branches are never exercised, so the PR's two distinctive code paths are untested.
- Minor/nit: `wrap` style inconsistency.

The implementation itself appears correct; the concerns are naming clarity and test completeness, not functional bugs.


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