Skip to content

Conversation

@KenLagos
Copy link

Description

  • Add check for implicit inputs for "If" node for destination argument index

  • Disable fusion if subgraph parent node is "If" or if graph has "If" node

…index

Disable fusion if subgraph parent node is "If" or if graph has "If" node
- move variables to inner scope
- remove std::string casting
@KenLagos
Copy link
Author

@apwojcik

Regarding testing the change, I have been testing with modelbench-ort and Amuse and haven't seen any regressions with the change but I'm unsure if that is good enough.

Are there any other things I should test with to confirm this change doesn't cause any regressions?

@KenLagos KenLagos requested a review from apwojcik October 22, 2025 14:26
@TedThemistokleous
Copy link
Collaborator

I don't understand the change here, sure it fixes the issue but we're just going to turn off fusions entirely then for if? Why were we getting an out of bound index in the first place?

@KenLagos KenLagos self-assigned this Oct 24, 2025
@KenLagos
Copy link
Author

KenLagos commented Oct 24, 2025

contined from resolved comment chain

Just to clarify, this change isn't a complete fix but an improvement to our current situation by changing our problem from a functional issue to a performance issue for models containing "If" nodes. Testing on my local machine suggests models with an "If" node just fail to run. It isn't an exhaustive test but a basic test model I made with an "If" node failed to run and would suggest "If" handling in most if not all models are probably currently broken. Resolving the wrong index error does help but still fails to run indicating another issue.

This change handles the incorrect index error by taking into account implicit inputs that are used by onnxruntime to determine the dst index when creating edges and adding it to the getSubGraph logic.

in graph.cc AddEdge() function
image

Added logic in GetSubGraph() migraphx_execution_provider.cc which avoids arg_index >= input_defs.size() error
image

What does the then_branch have in it? Should have its own list of operators or things when you click on it? Are you viewing this in netron?

I'm using netron.

Test "If" Model
image

The branches looks like below:

else:
image

then:
image

the failing lmx model is a bit more complicated and looks like.

image

else
image

then:
image

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.

3 participants