Skip to content

Fix nonzero for non-standard input layouts#5046

Open
ikalinic wants to merge 5 commits into
developfrom
ikalinic/nonzero-layout-fix
Open

Fix nonzero for non-standard input layouts#5046
ikalinic wants to merge 5 commits into
developfrom
ikalinic/nonzero-layout-fix

Conversation

@ikalinic

@ikalinic ikalinic commented Jul 8, 2026

Copy link
Copy Markdown
Member

Motivation

Fix the reference nonzero operator so it accepts and correctly reads non-standard input layouts such as transposed and broadcasted tensors.

Technical Details

Removed the standard-layout requirement from nonzero shape checking and changed compute-time element access to use the logical index from shape_for_each. Added reference and verify tests for transposed inputs, plus reference coverage for broadcasted inputs.

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.

@ikalinic
ikalinic requested review from a team and causten as code owners July 8, 2026 14:08
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5046      +/-   ##
===========================================
+ Coverage    92.78%   92.90%   +0.12%     
===========================================
  Files          596      603       +7     
  Lines        32011    32524     +513     
===========================================
+ Hits         29700    30214     +514     
+ Misses        2311     2310       -1     
Files with missing lines Coverage Δ
src/include/migraphx/op/nonzero.hpp 95.45% <100.00%> (-4.55%) ⬇️

... and 29 files with indirect coverage changes

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

Comment thread src/include/migraphx/op/nonzero.hpp Outdated
Comment thread src/include/migraphx/op/nonzero.hpp
Comment thread src/include/migraphx/op/nonzero.hpp Outdated
@gh-app-migraphx-bot-pr-write

gh-app-migraphx-bot-pr-write Bot commented Jul 8, 2026

Copy link
Copy Markdown
Test Batch New Rate (791bc4) Old Rate (e1e03e) Diff Status
torchvision-resnet50 64 1,428.98 2,693.41 -46.95% 🔴
torchvision-resnet50_fp16 64 6,568.04 6,415.71 2.37%
torchvision-densenet121 32 2,684.86 2,702.20 -0.64%
torchvision-densenet121_fp16 32 4,504.53 4,553.39 -1.07%
torchvision-inceptionv3 32 1,797.70 950.94 89.04% 🔆
torchvision-inceptionv3_fp16 32 2,820.20 2,269.97 24.24% 🔆
cadene-inceptionv4 16 821.87 538.17 52.72% 🔆
cadene-resnext64x4 16 783.58 459.28 70.61% 🔆
slim-mobilenet 64 8,382.68 8,429.89 -0.56%
slim-nasnetalarge 64 228.85 89.65 155.28% 🔆
slim-resnet50v2 64 3,162.27 1,563.17 102.30% 🔆
bert-mrpc-onnx 8 1,171.48 1,165.41 0.52%
bert-mrpc-tf 1 488.76 486.44 0.48%
pytorch-examples-wlang-gru 1 489.30 487.62 0.34%
pytorch-examples-wlang-lstm 1 384.51 392.17 -1.95%
torchvision-resnet50_1 1 753.22 771.94 -2.43%
cadene-dpn92_1 1 441.26 442.37 -0.25%
cadene-resnext101_1 1 366.08 364.35 0.48%
onnx-taau-downsample 1 401.04 400.76 0.07%
dlrm-criteoterabyte 1 32.42 32.68 -0.78%
dlrm-criteoterabyte_fp16 1 51.80 52.60 -1.51%
agentmodel 1 9,290.57 9,162.71 1.40%
unet_fp16 2 57.03 57.35 -0.55%
resnet50v1_fp16 1 933.86 1,017.75 -8.24% 🔴
resnet50v1_int8 1 943.46 937.30 0.66%
bert_base_cased_fp16 64 1,097.63 1,102.37 -0.43%
bert_large_uncased_fp16 32 345.62 346.79 -0.34%
bert_large_fp16 1 204.35 203.94 0.20%
distilgpt2_fp16 16 2,095.43 2,094.08 0.06%
yolov5s 1 559.88 560.76 -0.16%
tinyllama 1 45.83 45.87 -0.10%
vicuna-fastchat 1 44.02 44.20 -0.40%
whisper-tiny-encoder 1 413.32 415.10 -0.43%
whisper-tiny-decoder 1 410.23 409.89 0.08%
llama2_7b 1 20.82 20.96 -0.65%
qwen1.5-7b 1 23.58 23.66 -0.34%
phi3-3.8b 1 26.72 26.82 -0.37%
llama3-8b 1 21.74 21.88 -0.67%
whisper-large-encoder 1 10.17 10.21 -0.34%
whisper-large-decoder 1 106.63 103.92 2.61%
mistral-7b 1 22.51 23.89 -5.80% 🔴
FLUX.1-schnell 1 762.32 757.77 0.60%

Regressions detected 🔴

@gh-app-migraphx-bot-pr-write

gh-app-migraphx-bot-pr-write Bot commented Jul 8, 2026

Copy link
Copy Markdown
Test Status Result
bert-mrpc-onnx PASSED: MIGraphX meets tolerance
bert-mrpc-tf PASSED: MIGraphX meets tolerance
pytorch-examples-wlang-gru PASSED: MIGraphX meets tolerance
pytorch-examples-wlang-lstm PASSED: MIGraphX meets tolerance
dlrm-criteoterabyte PASSED: MIGraphX meets tolerance
agentmodel PASSED: MIGraphX meets tolerance
unet PASSED: MIGraphX meets tolerance
resnet50v1 PASSED: MIGraphX meets tolerance
bert_base_cased_fp16 PASSED: MIGraphX meets tolerance
bert_large_uncased_fp16 🔴 FAILED: MIGraphX is not within tolerance - check verbose output
bert_large PASSED: MIGraphX meets tolerance
yolov5s PASSED: MIGraphX meets tolerance
tinyllama PASSED: MIGraphX meets tolerance
vicuna-fastchat PASSED: MIGraphX meets tolerance
whisper-tiny-encoder PASSED: MIGraphX meets tolerance
whisper-tiny-decoder PASSED: MIGraphX meets tolerance
distilgpt2_fp16 PASSED: MIGraphX meets tolerance
llama2_7b PASSED: MIGraphX meets tolerance
qwen1.5-7b PASSED: MIGraphX meets tolerance
phi3-3.8b PASSED: MIGraphX meets tolerance
llama3-8b PASSED: MIGraphX meets tolerance
whisper-large-encoder PASSED: MIGraphX meets tolerance
whisper-large-decoder PASSED: MIGraphX meets tolerance
mistral-7b PASSED: MIGraphX meets tolerance
FLUX.1-schnell PASSED: MIGraphX meets tolerance

@ikalinic
ikalinic requested a review from pfultz2 July 9, 2026 13:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Fixes the reference implementation of the nonzero operator to correctly handle non-standard input layouts (e.g., transposed and broadcasted tensors), aligning compute-time element access with logical indexing.

Changes:

  • Removed the standard-layout constraint from nonzero shape validation and updated compute-time access to use logical indices from shape_for_each.
  • Added new reference + verify tests covering transposed inputs and reference coverage for broadcasted inputs.
  • Added a changelog entry under “Resolved issues”.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/include/migraphx/op/nonzero.hpp Removes standard-layout requirement and updates indexing logic to support non-standard layouts.
test/ref/nonzero.cpp Adds reference tests for transposed and broadcasted inputs.
test/verify/test_nonzero.cpp Adds verify coverage for transposed inputs.
CHANGELOG.md Documents the fix under resolved issues.

Comment thread src/include/migraphx/op/nonzero.hpp Outdated
Comment thread src/include/migraphx/op/nonzero.hpp Outdated
Comment on lines 56 to 60
@@ -56,8 +58,8 @@ struct nonzero
std::vector<std::vector<std::size_t>> vec_idx;
auto s = args.front().get_shape();
args.front().visit([&](auto v) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@ikalinic
ikalinic requested a review from bdevorem July 16, 2026 13:16
@TedThemistokleous
TedThemistokleous self-requested a review July 16, 2026 14:15
Comment thread src/include/migraphx/op/nonzero.hpp Outdated

@TedThemistokleous TedThemistokleous left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just need clarification on your test data. I think you have extra values that aren't checked / dont make sense.

Possible to also clean up the output assignment with a transform using migraphx ranges for front().size instead of keeping the raw loop.

Comment thread test/ref/nonzero.cpp
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.

5 participants