Skip to content

mlir-imp: resolve If/Loop body values that shadow the owner's output names locally - #1333

Open
amd-rmotiram wants to merge 1 commit into
ROCm:mainfrom
amd-rmotiram:fix/model-load-failures
Open

amd-rmotiram wants to merge 1 commit into
ROCm:mainfrom
amd-rmotiram:fix/model-load-failures

Conversation

@amd-rmotiram

Copy link
Copy Markdown

mlir-imp: fix import of If bodies whose outputs reuse the If's output names

Summary

The 12 whisper "merged" decoders (whisper-base and whisper-large-v3-turbo decoder_model_merged{,_bnb4,_int8,_q4,_quantized,_uint8}) fail during the ONNX -> MLIR import:

register_captured_alias: null Value for 'present.0.decoder.key'

Their If branches return values named the same as the If's own outputs. ONNX allows this: inside a branch, the branch's value shadows the outer name. The importer mishandled it in two places in morphizen/mlir-imp/src/mlir-graph.cpp.

Problem

flowchart TD
    A["Import If: output names declared<br/>(placeholders, no Value yet)"] --> B["Import branch bodies"]
    B --> C["Branch looks up present.0.decoder.key"]
    C --> D["Not local, found in parent<br/>as an empty placeholder"]
    D --> E["Treated as a capture<br/>register_captured_alias(name, null)"]
    E --> F["CHECK fails: null Value"]
    style F fill:#f8d7da,stroke:#c00

    G["Build main-graph name table<br/>(post-order: children first)"] --> H["Branch value claims the name"]
    H --> I["If output dropped (emplace keeps first)"]
    I --> J["Partitioning fails<br/>morphizen-ep.cpp:894 / :461"]
    style J fill:#f8d7da,stroke:#c00
Loading

The second failure only shows up once the first is fixed.

Solution

flowchart TD
    C["Branch looks up a name"] --> L{"Local?"}
    L -- Yes --> U["Use local value"]
    L -- No --> P{"In parent AND<br/>has a Value?"}
    P -- Yes --> R["Real capture"]
    P -- No --> S["Not found: branch creates its own<br/>value (shadows outer name)"]
    style R fill:#d4edda,stroke:#080
    style S fill:#d4edda,stroke:#080

    G["Build main-graph name table<br/>(pre-order: parent first)"] --> H["If claims its output names first"]
    H --> I["Same-named branch values don't override"]
    style I fill:#d4edda,stroke:#080
Loading
  • get_node_arg_index: an outer name only counts as a capture if it already has a Value. Genuine captures always do, because outer nodes are imported in topological order.
  • initialize_node_args_map: walk pre-order, matching populate_node_arg_indexes.
  • add_node: log a warning when a named input has no producer and is replaced by none. Before this change, that case asserted.

Testing (Strix Halo, gfx1151)

  • All 12 merged decoders now pass import. They then stop at a separate pipeline error: 'onnx.If' op failed to build output inits (fully-static output shapes required).
  • New test/numeric/tests/test_if_subgraph_names.py (then and else branches) and the existing test_dangling_outputs.py pass: 6/6. The new test fails without this fix.
  • Smoke: resnet50_fp16, mobilenetv3_fp32, mosaic_fp16 and whisper-base encoder are unchanged. whisper-base decoder_model.onnx crashes (0xC0000005) both with and without this change.

Known gaps

  • No end-to-end numeric check on the 12 real models yet, because the onnx.If static-shape error above blocks them.
  • Tests cover If only. Loop/Scan/nested-If shadowing uses the same code path but isn't directly tested, and there is no lit test.
  • A malformed graph referencing a never-produced outer value no longer asserts. It now gets the add_node warning and a none input. ORT's load-time validation normally rejects such models first.

@github-actions

Copy link
Copy Markdown

Thanks for opening a PR!

This project follows LLVM's incremental-development and AI-tool-use
guidance. See CONTRIBUTING.md
for the project workflow.

Before requesting review, please check that:

  1. The change is focused. Substantial work links the relevant issue
    or design discussion.
  2. The PR documents relevant test results and updates affected
    documentation.
  3. If AI tools provided substantial assistance, the description
    explains what was assisted and how it was validated, and commit
    trailers identify the tool. The contributor has reviewed and
    understands the result.

Reviewers are assigned through
CODEOWNERS where ownership
is configured.

…names locally

Outer NodeArgs without a Value (the enclosing op's own outputs) are no longer treated as captures, and the node-arg name map is built pre-order so the region op claims its result names before same-named body values. Fixes import of whisper merged decoders. Adds numeric test test_if_subgraph_names.py.

Co-authored-by: Cursor <cursoragent@cursor.com>
@amd-rmotiram
amd-rmotiram force-pushed the fix/model-load-failures branch from f724fee to f22e92a Compare October 26, 2026 21:15

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant