Fix dynamic-shape parse failure in Softplus and Softsign - #5136
Fix dynamic-shape parse failure in Softplus and Softsign#5136zhihuidu-amd wants to merge 3 commits into
Conversation
parse_softplus and parse_softsign build their broadcast literal with
multibroadcast{"out_lens", args[0]->get_shape().lens()}
and shape::lens() throws for a dynamic shape, so parsing any ONNX model
containing Softplus or Softsign aborts with
shape.cpp: lens: SHAPE: lens() called on a dynamic shape
as soon as an input dimension has min != max. A fixed dynamic dimension
({n,n}) does not reproduce it; a range as narrow as {95,96} does.
Both parsers now use add_common_op, whose documented purpose is exactly
this ("use add_common_op() to match input sizes when inputs may be either
static or dynamic"). parse_binary_op.cpp has the same multibroadcast
pattern but guards it with an explicit MIGRAPHX_THROW, so these two were
the outliers.
For static shapes the emitted graph is op-for-op identical; only the
instruction order changes, since the broadcast is now inserted next to
the add that consumes it rather than ahead of the exp/abs. The four
existing softplus/softsign tests construct their expected program by
hand and compare with EXPECT(p == prog), which is order-sensitive, so
they are reordered to match. No behaviour change.
Adds softplus_dyn_test and softsign_dyn_test, which parse the existing
.onnx fixtures with map_dyn_input_dims set to a non-fixed range - the
case that fails today and had no coverage. They reuse the existing
fixtures, and test/onnx/CMakeLists.txt globs parse/*.cpp, so no new
data files or build changes are needed.
|
Thank you for your contribution! Since this is an external pull request, a maintainer must review PR and add the "ok-to-test" label if it is approved for testing. |
The licensing CI check requires every modified file to carry the current
year in its copyright stamp, and reported:
8 Wrong year in license files
The two parsers were stamped 2015-2022 and the four existing tests
2015-2023; the two new tests inherited 2022 from the file they were
copied from. All eight now read 2015-2026.
Verified with the repository's own checker, tools/check_stamped.py, which
now reports that all files are properly stamped.
No functional change - this touches only the copyright line in each file.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5136 +/- ##
===========================================
+ Coverage 93.12% 93.13% +0.01%
===========================================
Files 625 625
Lines 33162 33246 +84
===========================================
+ Hits 30879 30961 +82
- Misses 2283 2285 +2
🚀 New features to boost your workflow:
|
|
CI status after the latest push, in case it helps whoever picks this up: 17 checks pass, 3 fail, and I don't believe any of the three is caused by this change. Details so you can judge rather than take my word for it. licensing — was a real failure, now fixed. The first push had the original copyright years (2015-2022 on the two parsers, 2015-2023 on the four existing tests), and the check correctly reported 8 Wrong year in license files. The second commit updates all eight to 2015-2026, verified with the repo's own tools/check_stamped.py ("Success: All files properly stamped"). That check is now green. Jenkins — network failure in the status callback, not a build failure. The reported error is: Error in windows-gpu — Chocolatey could not serve Visual Studio Build Tools. Failed to fetch results from V2 feed at 'https://community.chocolatey.org/api/v2/Packages(Id='visualstudio2022buildtools',...)' security_gate — expected. Missing 'ok-to-test' label; BLOCKED, the standard gate for an external contributor. This is what mergeable_state: blocked reflects. I'm deliberately not pushing empty commits to re-trigger CI, since a new commit drops the ok-to-test label and would only add churn. If a maintainer adds the label, the remaining jobs should re-run and the two infrastructure failures will most likely clear on their own. One thing worth repeating from the PR description, since it is the weakest part of the evidence: the 827-test ONNX suite run was executed against tag rocm-7.2.0, not against develop. I could not rebuild on develop locally — it needs a newer rocm-cmake than the ROCm available to me (Unknown CMake command "rocm_add_version_resource"). I checked transfer by inspection instead: all six modified files are byte-identical between the two revisions, and add_common_op's implementation is unchanged. The develop CI here is therefore the first real test of this patch on this base, and I'd rather flag that than have it assumed. |
CharlieL7
left a comment
CharlieL7
left a comment
There was a problem hiding this comment.
The changes look OK. There are a lot of unnecessary comments throughout the PR that should be made more terse or removed. I added comments for a few of them but there are more. Please update the comments.
What is the model that is hitting Softplus or Softsign with dynamic shapes? Also what type of dynamic shape. Dynamic batch for example?
Remove the explanatory comments CharlieL7 flagged and the others of the same kind. No code changes - only comments were touched. Kept: the two pre-existing formula comments that were in the parsers before this PR, and one line per new regression test naming what regressed.
2 participants
parse_softplus and parse_softsign build their broadcast literal with
and shape::lens() throws for a dynamic shape, so parsing any ONNX model containing Softplus or Softsign aborts with
as soon as an input dimension has min != max. A fixed dynamic dimension ({n,n}) does not reproduce it; a range as narrow as {95,96} does.
Both parsers now use add_common_op, whose documented purpose is exactly this ("use add_common_op() to match input sizes when inputs may be either static or dynamic"). parse_binary_op.cpp has the same multibroadcast pattern but guards it with an explicit MIGRAPHX_THROW, so these two were the outliers.
For static shapes the emitted graph is op-for-op identical; only the instruction order changes, since the broadcast is now inserted next to the add that consumes it rather than ahead of the exp/abs. The four existing softplus/softsign tests construct their expected program by hand and compare with EXPECT(p == prog), which is order-sensitive, so they are reordered to match. No behaviour change.
Adds softplus_dyn_test and softsign_dyn_test, which parse the existing .onnx fixtures with map_dyn_input_dims set to a non-fixed range - the case that fails today and had no coverage. They reuse the existing fixtures, and test/onnx/CMakeLists.txt globs parse/*.cpp, so no new data files or build changes are needed.
Motivation
Technical Details
Changelog Category
Add a
CHANGELOG.mdentry for any option other thanNot ApplicableFollow the LLVM AI Tool Use Policy for contributions using AI.