Repository navigation
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
This PR prevents optimizer fusions that would silently narrow DOUBLE constants into FLOAT attributes, which can change model outputs.
Changes:
- MatMulScaleFusion now refuses fusion when a DOUBLE scale cannot be represented losslessly as a float attribute.
- LayerNormFusion/SimplifiedLayerNormFusion require epsilon to be a scalar constant that round-trips exactly through float.
- Added regression tests covering lossy DOUBLE cases and confirming existing FLOAT/lossless DOUBLE fusions still work.
| File | Description |
|---|---|
| onnxruntime/test/optimizer/graph_transform_test_layernorm.cc | Adds tests ensuring LayerNorm fusions are rejected for lossy DOUBLE epsilon. |
| onnxruntime/test/optimizer/graph_transform_test.cc | Adds a MatMulScaleFusion test that rejects lossy DOUBLE scales; includes <cmath> for nextafter. |
| onnxruntime/core/optimizer/matmul_scale_fusion.cc | Adds lossless-check plumbing when extracting scalar constants as float for fusion attributes. |
| onnxruntime/core/optimizer/layer_norm_fusion.cc | Tightens epsilon handling to require a scalar initializer and lossless float round-trip before fusing. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Div and combined-scale alpha calculations can still narrow DOUBLE values, and one regression test does not exercise its intended path.
Review effort: Balanced
Findings: 1
Open (2)
Resolved since last review (3)
|
Dmitri Smirnov (@yuslepukhin) The latest commit addresses all review threads and passes the 16 targeted optimizer tests locally. The GitHub Actions workflows are currently waiting for maintainer approval. Could you approve them when convenient? |
|
The workflows for aadacfc are currently marked action_required and are waiting for approval. I also reproduced the failing WASM Release static-library configuration locally with WASM SIMD, RTTI disabled, and API exception catching enabled; the full build completed successfully through bundling_target with warnings treated as errors. Could a maintainer please approve and run the workflows? Thanks. |
|
Follow-up pushed in bf0854c. Cast-to-FLOAT epsilon constants now convert directly to float, avoiding double rounding for large integer initializers; exact DOUBLE epsilons remain accepted and lossy ones remain rejected. The three targeted LayerNorm tests pass in a clean training-enabled ORT build. |
Signed-off-by: Yuhx141 <155034285+Yuhx141@users.noreply.github.com>
Description
This change declines graph fusions when a DOUBLE constant cannot make the required round trip through a float attribute.
Exactly representable DOUBLE values and the existing FLOAT paths still fuse.
Motivation and Context
FusedMatMul and LayerNormalization store these values in float attributes. Narrowing a valid DOUBLE initializer changed model output.
Fixes #32961
Fixes #32962
Testing
Built onnxruntime_test_all with training enabled in Release mode.
Ran 7 focused tests covering the two new regressions plus existing FLOAT and exactly representable DOUBLE fusion paths; all passed.