Skip to content

Conversation

@KenLagos
Copy link

Description

Bug Fix: large integers incorrectly converted to -1 in IsUnsupportedOpMode() function

prevent casting of int64 to int32 in IsUnsupportedOpMode() function for "Slice" operator

Motivation and Context

Currently clc models in topaz underperform DML by 52x and the root cause is due to the "Slice" node being flagged as unsupported.

This change fixes an issue where start and end attributes for the slice node were being converted to -1 instead of their real int64 value. The -1 end or start value was causing migraphx/onnxruntime to incorrectly flag this node as unsupported.

Local testing on my machine suggests inference time goes from 90 seconds to 3 seconds which is massive boost in performance and closes the gap between DML and Migraphx performance considerably.

KenLagos and others added 3 commits October 23, 2025 17:40
…pMode() function

- prevent casting of int64 to int32 in IsUnsupportedOpMode() function for "Slice" operator
@Zhaeong
Copy link

Zhaeong commented Oct 23, 2025

This will need to be cherry-picked into wml-main after merging

@KenLagos KenLagos requested a review from apwojcik October 24, 2025 14:52
@KenLagos KenLagos self-assigned this Oct 24, 2025
@TedThemistokleous
Copy link
Collaborator

What in the slice check is being unsupported? Why are you modifying code that's doing index checks?

Copy link
Collaborator

@TedThemistokleous TedThemistokleous left a comment

Choose a reason for hiding this comment

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

The fix is in toVector call to use a larger size, its using int which may default to 32 bits on some systems. If you're seeing rollover that's likely the culprit

Either make this a larger type to avoid casting things down to int32 (since int-> unsigned) or use size_t/int64_t

std::vector<int> toVector(const ONNX_NAMESPACE::int64s& nums) {
  std::vector<int> result;
  int num = nums.size();
  for (int i = 0; i < num; ++i) {
    result.push_back(static_cast<int>(nums[i]));
  }

  return result;
}

Also don't remove the .at() bounds checks here. These shouldn't be removed for direct access [ ]

@TedThemistokleous TedThemistokleous merged commit 8dfad21 into ROCm:rocm7.1_internal_testing Oct 24, 2025
2 of 4 checks passed
@TedThemistokleous TedThemistokleous added the Upstream Changset that should be merged upstream to Microsoft/Onnxruntime label Oct 24, 2025
@TedThemistokleous
Copy link
Collaborator

@Zhaeong good to cherry-pick this off the tip of rocm7.1_internal_testing. I'll upstream this to OnnxRT

@TedThemistokleous
Copy link
Collaborator

Upstreamed here - microsoft#26403

@Zhaeong
Copy link

Zhaeong commented Oct 24, 2025

Merged to wml-main:
#189

@TedThemistokleous TedThemistokleous added the Promote Cherry-pick this change to latest ROCm internal branch label Oct 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Promote Cherry-pick this change to latest ROCm internal branch Upstream Changset that should be merged upstream to Microsoft/Onnxruntime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants