Skip to content

Have convolution_backwards error on indivisible groups and reference operator accumulate in double - #5248

Open
CharlieL7 wants to merge 5 commits into
developfrom
conv_backwards_ref_accumulator
Open

CharlieL7 wants to merge 5 commits into
developfrom
conv_backwards_ref_accumulator

Conversation

@CharlieL7

@CharlieL7 CharlieL7 commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Motivation

  • convolution_backwards was hitting accuracy errors with allclose due to reference accumulator inaccuracy. It was accumulating in the low precision types on the reference side.
  • Also fixed correctness for convolution_backwards for invalid groups settings.

Technical Details

  • Change accumulator to use double type on reference convolution_backwards.

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.

Follow the LLVM AI Tool Use Policy for contributions using AI.

@CharlieL7
CharlieL7 marked this pull request as ready for review September 11, 2026 17:48
@CharlieL7
CharlieL7 requested review from a team and causten as code owners September 11, 2026 17:48
Copilot AI lite review requested due to automatic review settings September 11, 2026 17:48

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.

🟢 Approval recommended

Remaining feedback is limited to minor comment clarifications.

Pull request overview

Fixes convolution_backwards validation and improves reference accumulation precision.

Changes:

  • Validates invalid groups and zero strides.
  • Uses higher-precision accumulators.
  • Adds regression tests and changelog coverage.
File summaries
File Summary Review notes
test/ref/convolution_backwards.cpp Numerical and grouped-convolution regression tests No issues
test/op_shape_test.cpp Invalid stride and group tests Nit: clarify the out-of-range group mapping
src/include/migraphx/op/convolution_backwards.hpp Accumulation and validation fixes Nit: clarify the invalid-group failure rationale
CHANGELOG.md Documents the resolved issue No issues
Review details

Suppressed comments (2)

src/include/migraphx/op/convolution_backwards.hpp:102

  • The rationale here describes the failure as leaving channels unread, but compute() uses group_id = w / (wei_n / group): with a non-divisible channel count, the trailing w values map to group_id >= group, so idx_out goes past the output channel dimension and can write/read out of bounds. Update the comment to document the actual invalid-group failure this check prevents.
        // compute() walks a group's input channels as one contiguous block of
        // weights_channels / group, so an inexact division leaves part of every group unread.

test/op_shape_test.cpp:1005

  • This test comment has the same incorrect failure description: integer division does not simply leave part of each group unread; the trailing input channel is assigned to an invalid group and makes idx_out exceed the output channel bounds. Please describe the out-of-range group mapping instead.
// compute() splits the input channels into equal per-group blocks, so a group that does not
// divide them would leave part of every group unread.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@eddieliao eddieliao 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.

Overall LGTM; minor nitpick with changelog entry.

Comment thread CHANGELOG.md Outdated
@CharlieL7
CharlieL7 requested a review from pfultz2 September 15, 2026 19:21
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