Skip to content

Fix fuse_pad_into_conv incorrectly fusing when Pad contains negative values#2841

Open
Copilot wants to merge 3 commits into
mainfrom
copilot/fix-negative-pad-values
Open

Fix fuse_pad_into_conv incorrectly fusing when Pad contains negative values#2841
Copilot wants to merge 3 commits into
mainfrom
copilot/fix-negative-pad-values

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 4, 2026

The fuse_pad_into_conv optimization would fuse a Pad node into Conv even when pads contained negative values (valid for Pad as crop operations, but invalid for Conv's pads attribute), producing an invalid model.

Changes

  • _fuse_pad_into_conv.py: Added a guard in _FuseConvPadBase.check() to bail out when any computed pad value is negative, preventing fusion that would produce an invalid Conv node:
    if any(p < 0 for p in self._pads_list):
        return check_result.fail(f"{pads.name} must not contain negative values.")
  • _fuse_pad_into_conv_test.py: Added a test case to test_unsupported_fuse_pad_into_conv verifying that fusion is rejected when spatial pads are negative.
Original prompt

This section details on the original issue you should resolve

<issue_title>fuse_pad_into_conv optimization incorrectly fuses when 'pads' contain negative values</issue_title>
<issue_description>converting the attached ONNX model with the following script:

from onnxscript import ir
from onnxscript.optimizer import optimize

ir_model = ir.load("input.onnx")
optimized_model = optimize(ir_model)
ir.save(optimized_model, "output.onnx")

results in the following output model:

Image

The Conv layer has negative pad values, which is not allowed per spec. This optimization should check for negative pad values.

input.onnx.zip

versions of relevant packages:

ml_dtypes         0.5.4
onnx              1.20.1
onnx-ir           0.1.16
onnxscript        0.6.2
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix optimization to handle negative pad values in Conv layer Fix fuse_pad_into_conv incorrectly fusing when Pad contains negative values Mar 4, 2026
@bas-aarts
Copy link
Copy Markdown

What is the reason the PR cannot be merged? It's a rathe simple fix.

@justinchuby justinchuby requested a review from Copilot June 1, 2026 16:21
@justinchuby justinchuby marked this pull request as ready for review June 1, 2026 16:22
Copy link
Copy Markdown
Contributor

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 fixes an invalid optimization in the rewriter where Pad was being fused into Conv even when Pad.pads contained negative values (crop semantics), which would produce an invalid Conv node (negative pads are not allowed for Conv).

Changes:

  • Add a guard in _FuseConvPadBase.check() to reject fusion when any computed pad value is negative.
  • Add a regression test ensuring fusion is rejected when spatial pad values are negative.

Reviewed changes

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

File Description
onnxscript/rewriter/rules/common/_fuse_pad_into_conv.py Prevents PadConv fusion when computed pads contain negative values, avoiding invalid Conv attributes.
onnxscript/rewriter/rules/common/_fuse_pad_into_conv_test.py Adds coverage for the negative-padding (crop) case to ensure the fusion is correctly rejected.

@titaiwangms titaiwangms self-requested a review June 1, 2026 16:30
@github-project-automation github-project-automation Bot moved this from Todo to Done in ONNX Script Review Board Jun 1, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.64%. Comparing base (8fdb1e0) to head (94112d5).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2841   +/-   ##
=======================================
  Coverage   72.64%   72.64%           
=======================================
  Files         259      259           
  Lines       31652    31655    +3     
  Branches     2980     2981    +1     
=======================================
+ Hits        22994    22997    +3     
  Misses       7649     7649           
  Partials     1009     1009           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

fuse_pad_into_conv optimization incorrectly fuses when 'pads' contain negative values

5 participants