fix(pnnx): handle denormal float literals in pass_ncnn expression expansion#6748
fix(pnnx): handle denormal float literals in pass_ncnn expression expansion#6748crystarm wants to merge 1 commit into
Conversation
crystarm
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses a pnnx -> ncnn conversion failure in pass_ncnn when pnnx.Expression contains denormal/subnormal float literals (e.g. -4.928072e-40) by replacing std::stof with a strtof-based parser and adding a regression test.
Changes:
- Implemented a
strtof-based float literal parser to accept underflow/subnormal values and reject true overflow. - Added explicit error logging when literal parsing fails during expression expansion.
- Added and registered a regression test to ensure
pass_ncnnproduces outputs for expressions containing denormal literals.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/pnnx/src/pass_ncnn/expand_expression.cpp | Replaces std::stof scalar parsing with a strtof-based parser and adds error logging on parse failure. |
| tools/pnnx/tests/ncnn/test_ncnn_expression_denorm.py | Adds a regression test that exports a TorchScript model containing denormal float literals and verifies pass_ncnn output + inference correctness. |
| tools/pnnx/tests/ncnn/CMakeLists.txt | Registers the new denormal-expression regression test in the ncnn test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static bool parse_float_noexcept(const std::string& t, float& f) | ||
| { | ||
| errno = 0; | ||
|
|
||
| char* endptr = 0; | ||
| f = strtof(t.c_str(), &endptr); | ||
|
|
||
| if (endptr == t.c_str()) | ||
| return false; | ||
|
|
||
| // strtof reports ERANGE for both overflow and underflow. | ||
| // Accept subnormal/underflow values and reject only true overflow. | ||
| if (errno == ERANGE && (f == HUGE_VALF || f == -HUGE_VALF)) | ||
| return false; | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Thanks for the catch. You're right that strtof accepts a valid prefix unless endptr is checked against the end of the string.
One note: this is not a new behavior introduced by this PR. The previous code used std::stof without checking pos, which also accepts a valid prefix and ignores trailing characters.
This PR is intentionally scoped to avoiding std::out_of_range on denormal/underflow literals in pass_ncnn. I agree that full-token validation is worth tightening, but I'd prefer to handle that separately to avoid mixing the denormal fix with a broader parser behavior change.
| # export torchscript | ||
| mod = torch.jit.trace(net, (x, y)) | ||
| mod.save("test_ncnn_expression_denorm.pt") | ||
|
|
||
| # torchscript to pnnx | ||
| if ( | ||
| os.system( | ||
| "../../src/pnnx test_ncnn_expression_denorm.pt inputshape=[1,4,8,8],[1,4,8,8]" | ||
| ) | ||
| != 0 | ||
| ): | ||
| return False | ||
|
|
||
| # ensure pass_ncnn produced output files | ||
| if not os.path.exists("test_ncnn_expression_denorm.ncnn.param"): | ||
| return False | ||
| if not os.path.exists("test_ncnn_expression_denorm.ncnn.bin"): | ||
| return False | ||
|
|
||
| # ensure the test model really contains denormalized literal expression | ||
| if not os.path.exists("test_ncnn_expression_denorm.pnnx.param"): | ||
| return False | ||
| with open("test_ncnn_expression_denorm.pnnx.param", "r", encoding="utf-8") as f: |
There was a problem hiding this comment.
Good point, thanks. Stale generated files can make this kind of test less robust.
For this PR I followed the existing tools/pnnx/tests/ncnn test style and kept the regression focused on the denormal-expression failure path. The test already checks the pnnx command result, verifies that the expected ncnn outputs are produced, confirms that the generated pnnx param still contains the denormal expression, and compares inference results.
I agree that cleanup/temp-directory isolation would make the tests more robust, but I think that is better handled as a broader test-suite cleanup rather than in this focused regression fix.
|
@nihui |
Related to #6614.
Summary
This PR fixes a
pnnxconversion failure whenpnnx.Expressioncontains denormal float literals (e.g.-4.928072e-40).Root cause
tools/pnnx/src/pass_ncnn/expand_expression.cppusedstd::stofto parse scalar literals.For denormal/underflow values,
std::stofmay throwstd::out_of_rangeon some platforms/libstdc++ implementations, which can interruptpass_ncnnand lead to missing.ncnn.param/.binoutputs.Changes
std::stofparsing inpass_ncnn/expand_expression.cppwith astrtof-based noexcept parser.tools/pnnx/tests/ncnn/test_ncnn_expression_denorm.pytools/pnnx/tests/ncnn/CMakeLists.txtValidation
python3 -m py_compile tools/pnnx/tests/ncnn/test_ncnn_expression_denorm.pyCompatibility
No API or model format changes.
Behavior for normal literals is unchanged; denormal literals no longer trigger this failure path.