Skip to content

Conversation

@RajBarshikar
Copy link

Motivation

This PR implements the missing ONNX operators SplitToSequence and ConcatFromSequence to enable support for modern generative models, specifically the FLUX image generation pipeline, which relies on these operators for its architecture.

Previously, these operators were blocking the execution of FLUX on MIGraphX because the compiler lacks a native dynamic Sequence data type. This PR introduces a static unrolling strategy to support these operators when the split configuration is known at compile-time.

Key Goals:

  • Enable SplitToSequence by statically slicing tensors into a fixed Tuple of outputs.
  • Enable ConcatFromSequence by accepting a Tuple input and delegating to the existing concat operator.
  • Resolve the blockage for models (like FLUX) that use these ops for architectural splits rather than dynamic data processing.

Technical Details

Since MIGraphX does not currently support dynamic sequences at runtime, this implementation adheres to a Static Constraint:

  1. SplitToSequence (src/onnx/parse_split_to_sequence.cpp):

    • Enforces that the split input must be a Constant.
    • Calculates split points at compile-time.
    • Generates explicit slice instructions for each chunk.
    • Returns the results as a Tuple (using the identity instruction with multiple inputs) instead of a dynamic Sequence.
    • Limitation: Throws a parse error if the split input is dynamic (variable at runtime).
  2. ConcatFromSequence (src/onnx/parse_concat_from_sequence.cpp):

    • Accepts a Tuple instruction as input (matching the output of SplitToSequence).
    • Extracts the underlying instructions via .inputs().
    • Supports the new_axis attribute by automatically injecting unsqueeze ops where required.
    • Delegates the final operation to the standard concat operator.

Validation:
Verified using a local ONNX test pipeline (Tensor -> SplitToSequence -> ConcatFromSequence -> Tensor) on the Reference backend to ensure data integrity is preserved.

Changelog Category

Add a CHANGELOG.md entry for any option other than Not Applicable

  • Added: New functionality.
  • Changed: Changes to existing functionality.
  • Removed: Functionality or support that has been removed. (Compared to a previous release)
  • Optimized: Component performance that has been optimized or improved.
  • Resolved Issues: Known issues from a previous version that have been resolved.
  • Not Applicable: This PR is not to be included in the changelog.

@RajBarshikar RajBarshikar requested review from a team and causten as code owners January 26, 2026 07:35

// 5. Bundle as a Tuple (The Static Sequence)
// We use the "identity" operator with multiple inputs to represent a tuple
return info.add_instruction(make_op("identity"), outputs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The identity operator does not create a tuple. The additional arguments to identity are ignored.


if(new_axis == 1)
{
std::vector<instruction_ref> unsqueezed_inputs;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this intermediate vector and just modify it in place.

@pfultz2
Copy link
Collaborator

pfultz2 commented Jan 26, 2026

Unit tests need to added for this,

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements ONNX operators SplitToSequence and ConcatFromSequence with a static unrolling strategy to support the FLUX image generation pipeline. Since MIGraphX lacks native dynamic sequence support, the implementation requires the split configuration to be compile-time constant and represents sequences as tuples of tensor instructions.

Changes:

  • Added parse_split_to_sequence.cpp to split tensors into fixed-size chunks when split configuration is constant
  • Added parse_concat_from_sequence.cpp to concatenate tuple inputs back into a single tensor with optional unsqueeze support
  • Updated CHANGELOG.md to document the new functionality

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.

File Description
src/onnx/parse_split_to_sequence.cpp Implements static SplitToSequence operator that slices input tensor into multiple outputs based on constant split configuration
src/onnx/parse_concat_from_sequence.cpp Implements ConcatFromSequence operator that concatenates sequence elements with optional new_axis support
CHANGELOG.md Documents addition of SplitToSequence and ConcatFromSequence operators with static support limitation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1 to 81
/*
* The MIT License (MIT)
*
* Copyright (c) 2015-2025 Advanced Micro Devices, Inc. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
#include <migraphx/onnx/op_parser.hpp>
#include <migraphx/onnx/checks.hpp>
#include <migraphx/instruction.hpp>
#include <migraphx/ranges.hpp>
#include <migraphx/stringutils.hpp>
#include <migraphx/make_op.hpp>

namespace migraphx {
inline namespace MIGRAPHX_INLINE_NS {
namespace onnx {

struct parse_concat_from_sequence : op_parser<parse_concat_from_sequence>
{
std::vector<op_desc> operators() const
{
return {{"ConcatFromSequence"}};
}

instruction_ref parse(const op_desc& /*opd*/,
const onnx_parser& /*parser*/,
const onnx_parser::node_info& info,
std::vector<instruction_ref> args) const
{
auto sequence_inst = args[0];
auto inputs = sequence_inst->inputs();

int64_t axis = 0;
if(contains(info.attributes, "axis"))
{
axis = info.attributes.at("axis").i();
}

int64_t new_axis = 0;
if(contains(info.attributes, "new_axis"))
{
new_axis = info.attributes.at("new_axis").i();
}

if(new_axis == 1)
{
std::vector<instruction_ref> unsqueezed_inputs;
unsqueezed_inputs.reserve(inputs.size());
for(auto& input : inputs)
{
unsqueezed_inputs.push_back(
info.add_instruction(make_op("unsqueeze", {{"axes", {axis}}}), input)
);
}
inputs = unsqueezed_inputs;
}

return info.add_instruction(make_op("concat", {{"axis", axis}}), inputs);
}
};

} // namespace onnx
} // namespace MIGRAPHX_INLINE_NS
} // namespace migraphx
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test files have been added for the ConcatFromSequence operator. The codebase has comprehensive test coverage for ONNX operators (see test/onnx/parse/ directory), and new operators should include test files following the same pattern. At minimum, tests should cover: basic functionality with multiple inputs, new_axis=0 and new_axis=1, negative axis values, and integration with SplitToSequence.

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +128
// 5. Bundle as a Tuple (The Static Sequence)
// We use the "identity" operator with multiple inputs to represent a tuple
return info.add_instruction(make_op("identity"), outputs);
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The identity operator with multiple inputs is not the correct pattern for returning multiple outputs. Instead, this function should return the outputs vector directly. The pattern for multi-output ONNX operators is to return std::vector<instruction_ref>, as seen in parse_split.cpp:149-159 and parse_dropout.cpp:37-49.

Copilot uses AI. Check for mistakes.
Comment on lines 72 to 81
if(not split_arg->can_eval())
{
MIGRAPHX_THROW("PARSE_SPLIT_TO_SEQUENCE: 'split' input must be constant");
}

auto split_literal = split_arg->eval();
// Case A1: 'split' is a list (1D Tensor)
if(split_literal.get_shape().elements() > 1)
{
split_literal.visit([&](auto s) { split_lengths.assign(s.begin(), s.end()); });
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error checking pattern is inconsistent with the codebase. Instead of using can_eval() followed by MIGRAPHX_THROW, use check_arg_empty() as shown in parse_split.cpp:107. Replace lines 72-75 with: auto split_literal = split_arg->eval(); check_arg_empty(split_literal, "PARSE_SPLIT_TO_SEQUENCE: 'split' input must be constant");

Suggested change
if(not split_arg->can_eval())
{
MIGRAPHX_THROW("PARSE_SPLIT_TO_SEQUENCE: 'split' input must be constant");
}
auto split_literal = split_arg->eval();
// Case A1: 'split' is a list (1D Tensor)
if(split_literal.get_shape().elements() > 1)
{
split_literal.visit([&](auto s) { split_lengths.assign(s.begin(), s.end()); });
auto split_literal = split_arg->eval();
check_arg_empty(split_literal,
"PARSE_SPLIT_TO_SEQUENCE: 'split' input must be constant");
// Case A1: 'split' is a list (1D Tensor)
if(split_literal.get_shape().elements() > 1)
{
split_literal.visit([&](auto s) { split_lengths.assign(s.begin(), s.end()); });
{
split_literal.visit([&](auto s) { split_lengths.assign(s.begin(), s.end()); });

Copilot uses AI. Check for mistakes.
Comment on lines 47 to 63
// 1. Get the 'axis' attribute (Default 0)
int64_t axis = 0;
if(contains(info.attributes, "axis"))
{
axis = info.attributes.at("axis").i();
}

// 2. Get the 'keepdims' attribute (Default 1)
int64_t keepdims = 1;
if(contains(info.attributes, "keepdims"))
{
keepdims = info.attributes.at("keepdims").i();
}

auto input = args[0];
auto input_shape = input->get_shape();
auto dim_size = input_shape.lens()[axis];
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing axis normalization using tune_axis to handle negative axis values. Other operators in the codebase (e.g., parse_split.cpp:179) use tune_axis(input_shape.ndim(), axis, opd.onnx_name) to normalize negative axis values to positive indices. This should be added after getting the axis attribute and before using it.

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +54
int64_t axis = 0;
if(contains(info.attributes, "axis"))
{
axis = info.attributes.at("axis").i();
}
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing axis normalization using tune_axis to handle negative axis values. Similar operators (e.g., parse_split.cpp:179) use tune_axis(input_shape.ndim(), axis, opd.onnx_name) to normalize negative axis values. This should be added after getting the axis attribute and before using it in the unsqueeze and concat operations.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Onnx spec does say that the axis is [-r, r - 1] so tune_axis needed here.

#include <migraphx/instruction.hpp>
#include <migraphx/ranges.hpp>
#include <migraphx/stringutils.hpp>
#include <migraphx/make_op.hpp>
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing required include #include <migraphx/tune_axis.hpp> which is needed for axis normalization. This header should be added if the suggestion to use tune_axis is implemented.

Suggested change
#include <migraphx/make_op.hpp>
#include <migraphx/make_op.hpp>
#include <migraphx/tune_axis.hpp>

Copilot uses AI. Check for mistakes.
Comment on lines 1 to 134
/*
* The MIT License (MIT)
*
* Copyright (c) 2015-2025 Advanced Micro Devices, Inc. All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
#include <migraphx/onnx/op_parser.hpp>
#include <migraphx/onnx/checks.hpp>
#include <migraphx/instruction.hpp>
#include <migraphx/ranges.hpp>
#include <migraphx/stringutils.hpp>
#include <migraphx/make_op.hpp>

namespace migraphx {
inline namespace MIGRAPHX_INLINE_NS {
namespace onnx {

struct parse_split_to_sequence : op_parser<parse_split_to_sequence>
{
std::vector<op_desc> operators() const
{
return {{"SplitToSequence"}};
}

instruction_ref parse(const op_desc& /*opd*/,
const onnx_parser& /*parser*/,
const onnx_parser::node_info& info,
std::vector<instruction_ref> args) const
{
// 1. Get the 'axis' attribute (Default 0)
int64_t axis = 0;
if(contains(info.attributes, "axis"))
{
axis = info.attributes.at("axis").i();
}

// 2. Get the 'keepdims' attribute (Default 1)
int64_t keepdims = 1;
if(contains(info.attributes, "keepdims"))
{
keepdims = info.attributes.at("keepdims").i();
}

auto input = args[0];
auto input_shape = input->get_shape();
auto dim_size = input_shape.lens()[axis];
std::vector<int64_t> split_lengths;

// 3. Determine Split Lengths
// Case A: 'split' input is provided
if(args.size() > 1)
{
auto split_arg = args[1];
// STATIC CHECK: The split configuration MUST be constant
if(not split_arg->can_eval())
{
MIGRAPHX_THROW("PARSE_SPLIT_TO_SEQUENCE: 'split' input must be constant");
}

auto split_literal = split_arg->eval();
// Case A1: 'split' is a list (1D Tensor)
if(split_literal.get_shape().elements() > 1)
{
split_literal.visit([&](auto s) { split_lengths.assign(s.begin(), s.end()); });
}
// Case A2: 'split' is a scalar (equal splits)
else
{
int64_t split_val = 0;
split_literal.visit([&](auto s) { split_val = s.front(); });

// Logic from ONNX spec for scalar split
int64_t num_chunks = dim_size / split_val;
split_lengths.resize(num_chunks, split_val);
int64_t remainder = dim_size - (num_chunks * split_val);
if (remainder > 0) split_lengths.push_back(remainder);
}
}
// Case B: 'split' input is missing (Default: Split into size 1)
else
{
split_lengths.resize(dim_size, 1);
}

// 4. Generate the Slice Instructions
std::vector<instruction_ref> outputs;
int64_t start_index = 0;

for(auto len : split_lengths)
{
int64_t end_index = start_index + len;

auto slice_op = make_op("slice", {{"axes", {axis}},
{"starts", {start_index}},
{"ends", {end_index}}});

auto slice_ins = info.add_instruction(slice_op, input);

// Handle keepdims=0 (Squeeze)
if(keepdims == 0)
{
slice_ins = info.add_instruction(make_op("squeeze", {{"axes", {axis}}}), slice_ins);
}

outputs.push_back(slice_ins);
start_index = end_index;
}

// 5. Bundle as a Tuple (The Static Sequence)
// We use the "identity" operator with multiple inputs to represent a tuple
return info.add_instruction(make_op("identity"), outputs);
}
};

} // namespace onnx
} // namespace MIGRAPHX_INLINE_NS
} // namespace migraphx
Copy link

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No test files have been added for the SplitToSequence operator. The codebase has comprehensive test coverage for ONNX operators (see test/onnx/parse/ directory), and new operators should include test files following the same pattern. At minimum, tests should cover: basic functionality, scalar vs vector split values, keepdims=0 and keepdims=1, negative axis values, and error cases (dynamic split input).

Copilot uses AI. Check for mistakes.
RajBarshikar and others added 5 commits January 27, 2026 11:45
Co-authored-by: Paul Fultz II <paul.fultz@amd.com>
Co-authored-by: Paul Fultz II <paul.fultz@amd.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 11.32075% with 47 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/onnx/parse_split_to_sequence.cpp 8.33% 33 Missing ⚠️
src/onnx/parse_concat_from_sequence.cpp 17.65% 14 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4571      +/-   ##
===========================================
- Coverage    92.22%   92.07%   -0.15%     
===========================================
  Files          564      569       +5     
  Lines        27335    27511     +176     
===========================================
+ Hits         25209    25329     +120     
- Misses        2126     2182      +56     
Files with missing lines Coverage Δ
src/onnx/parse_concat_from_sequence.cpp 17.65% <17.65%> (ø)
src/onnx/parse_split_to_sequence.cpp 8.33% <8.33%> (ø)

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants