[MIGraphX] Fix handling of Loop/If/Scan subgraphs and Loops with non-constant / huge maximum iteration count - #31978
Conversation
The `GraphPartitioner` is recursing bottom-up into Loop/If/Scan subgraphs, and the EP then creates new `MGXKernel_subgraph_*` ops for them inside the body. Later during compilation, MIGraphX's parser recurses into the body, finds the new unknown ops and fails with Unknown operator: MGXKernel_subgraph_tf2onnx_... To avoid that, early return in these cases and consider them unsupported, falling back to the CPU instead for them. Fixes microsoft#31713
… MIGraphX execution provider Consider such cases as unsupported ops, as MIGraphX clamps them to 65535 iterations, which a) does not work for non-constant iterations, b) doesn't give the desired result for higher iteration counts, and c) uses huge amounts of memory.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree company="Centricular" |
|
Doesn't look like |
Sebastian Dröge (sdroege)
changed the title
Sebastian Dröge (sdroege)
changed the title
Sebastian Dröge (sdroege)
changed the title
Tianlei Wu (tianleiwu)
left a comment
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
The control-flow body partitioning and recursive support checks look sound. Requesting changes because the Loop upper bound is not enforced for valid typed-field or external-data initializers, leaving the wrong-result and excessive-memory case reachable. Please also add focused capability tests for supported/unsupported nested bodies and trip counts around 65,535, including both raw_data and int64_data encodings.
| if (!canEvalNodeArgument(graph_viewer, node, {0}, m_input_nodes)) { | ||
| return true; | ||
| } | ||
| // constant-foldable but not a readable initializer, accept it as a best effort |
There was a problem hiding this comment.
Blocking: This best-effort path still accepts large constant trip counts when the initializer is not encoded as exactly eight bytes of raw_data. ONNX can store an INT64 initializer in int64_data or external data; canEvalNodeArgument() returns true for any initializer without decoding its value. For example, int64_data: 100000 reaches this branch, is assigned to MIGraphX, and is then clamped to 65,535, preserving the wrong-result/excessive-memory behavior this change aims to prevent. Please decode all scalar initializer representations (for example via ORT's Initializer or utils::UnpackTensor) and reject the Loop unless the actual value is available and <= 65,535. That also avoids the alignment-unsafe reinterpret_cast<int64_t*> read in the raw-data branch.
2 participants
Description
See individual commits. This fixes two separate issues in the MIGraphX execution provider:
Fix handling of Loop with non-constant, dynamic or huge iterations in MIGraphX execution provider
Consider such cases as unsupported ops, as MIGraphX clamps them to
65535 iterations, which a) does not work for non-constant iterations, b)
doesn't give the desired result for higher iteration counts, and c) uses
huge amounts of memory.
Fix handling of Loop/If/Scan subgraphs in MIGraphX execution provider
The
GraphPartitioneris recursing bottom-up into Loop/If/Scansubgraphs, and the EP then creates new
MGXKernel_subgraph_*ops forthem inside the body. Later during compilation, MIGraphX's parser
recurses into the body, finds the new unknown ops and fails with
Unknown operator: MGXKernel_subgraph_tf2onnx_...
To avoid that, early return in these cases and consider them
unsupported, falling back to the CPU instead for them.
Motivation and Context
This fixes #31713 but the model there still does not load correctly because of what I think is a bug in MIGraphX itself (ROCm/AMDMIGraphX#5126).