[Mlir-commits] [mlir] [mlir-c] Add slice analysis and topological sort (PR #206550)

[Maksim Levental]

makslevental wrote:

**Adversarial test review — test honesty & coverage** (consolidated; supersedes my two earlier duplicate review comments, which I'm deleting).

Note upfront: the earlier reviews were written against the original `copySlice` "fill up to `n`, return total" design. That API has since been redesigned into separate `mlir*Size` query functions + `void` fill functions that always write all entries. So the earlier concerns about the untested truncation case (`n < total`), writing past `n`, and the `n == 0` size-query path are now **moot** — each of the three size queries (`mlirGetForwardSliceSize`, `mlirGetBackwardSliceSize`, `mlirRegionGetBlocksSortedByDominanceSize`) is now exercised. What remains below is what still applies to the current code.

## Test honesty

### 1. (High) `testTopologicalSort` blocks-by-dominance assertions are vacuous
`mlir/test/CAPI/ir.c` (~L3515-3522):
```c
intptr_t numBlocks = mlirRegionGetBlocksSortedByDominanceSize(gRegion);
assert(numBlocks == 4);
MlirBlock blocks[4];
mlirRegionGetBlocksSortedByDominance(gRegion, blocks);
assert(mlirBlockEqual(blocks[0], entry));
assert(mlirBlockEqual(blocks[3], bb3));
```
The `@g` CFG is a diamond whose **textual block order already equals its dominance order** (`entry, ^bb1, ^bb2, ^bb3`). `getBlocksSortedByDominance` is an RPO traversal from the entry, which for this diamond yields exactly that same order. So the assertions pin nothing dominance-specific:
- `blocks[0] == entry` is tautological (the entry is by definition the region's first block);
- `blocks[3] == bb3` is just "the last textual block is last";
- `blocks[1]`/`blocks[2]` are never checked; `numBlocks == 4` only checks the count.

**A no-op that emits blocks in region/textual order passes this test identically to the real sort.** To make it honest: parse a region whose textual order differs from dominance order (e.g. emit `^bb3` textually before `^bb1`/`^bb2`), then assert the full 4-element reordered sequence so a plain region-order copy fails. (The `topologicalSort`-of-ops portion right below is the model to follow — it passes ops reversed and pins the exact restored order with `CHECK-NEXT`, so a pass-through no-op would fail. Good, non-vacuous test.)

## Coverage gaps

### 2. (Medium) `mlirTopologicalSort` dedup behavior untested and undocumented
`Analysis.cpp` funnels `ops` through a `SetVector`, so duplicate inputs are silently deduplicated and `sorted` receives fewer than `nOps` entries — leaving trailing slots unwritten, with no count returned to the caller (it's `void`, unlike the other three entry points). The test passes 3 distinct ops and reads back 3; the dedup case is never hit. A wrong impl that instead wrote `nOps` entries (stale/garbage tail) would pass. Either return the written count (consistent with the other three APIs) or document in the header that `ops` must be duplicate-free; and add a duplicate-input test.

### 3. (Medium) `mlirTopologicalSort` only covers the trivial single-block case
The header advertises "taking region semantics into account" and "input operations need not all belong to the same block", but all three inputs are in one block and none has nested regions. Neither documented behavior is exercised.

### 4. (Medium) Backward-slice `-1` failure contract unverified
`mlirGetBackwardSliceSize` documents "negative value if the backward slice could not be computed" and `mlirGetBackwardSlice` has a discard-on-failure branch; neither is ever observed. A regression that never returns `-1`, or that inverted the `failed(...)` check, would not be caught. The failure requires a Value that is neither an OpResult nor a BlockArgument, which is hard/near-impossible to trigger from ordinary parsed IR — so if no trigger is feasible, at least add a header/comment noting the path is effectively unreachable rather than leaving the contract silently unverified.

### 5. (Low–Medium) Slice result order not pinned; empty slices untested; filter `userData` not threaded through in any test
- Both slices are documented "written (in slice order)" but the tests use `CHECK-DAG` (membership + count only). An impl returning the correct set in the wrong order passes. If order is a real contract, use ordered `CHECK-NEXT` for at least one slice.
- Empty slices are never tested — e.g. forward slice of `return` (size 0), or backward slice of the `addi` whose only operands are block args (size 0). The fill loops are never run with a 0-length result.
- The `filter` callbacks (`sliceFilterExcludeSubi`/`Muli`) ignore `userData`, and every call passes `NULL`. Nothing verifies the `filterUserData` pointer is actually threaded to the callback (contrast `testReplaceUsesWithIf`, which does verify its `userData`). A regression dropping/mis-threading `filterUserData` would go unnoticed.

## Well-covered (no action)
- `testForwardSlice` / `testBackwardSlice`: `assert(count == N)` plus `CHECK-DAG` names bind both size and membership; the filtered cases (`count == 1`, `CHECK-NEXT` + `CHECK-NOT: slice:`) correctly prove the frontier filter drops the excluded op and anything only reachable through it. A no-op filter would fail. Honest.
- `mlirTopologicalSort` ops chain: reversed input + ordered `CHECK-NEXT` defeats a pass-through no-op. Honest.

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