-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(pnnx): handle denormal float literals in pass_ncnn expression expansion #6748
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| # Copyright 2026 Tencent | ||
| # SPDX-License-Identifier: BSD-3-Clause | ||
|
|
||
| import os | ||
|
|
||
| import torch | ||
| import torch.nn as nn | ||
|
|
||
|
|
||
| class Model(nn.Module): | ||
| def __init__(self): | ||
| super(Model, self).__init__() | ||
|
|
||
| def forward(self, x, y): | ||
| out0 = x * -4.928072e-40 + y | ||
| out1 = out0 * -4.881353e-40 + y | ||
| return out0, out1 | ||
|
|
||
|
|
||
| def test(): | ||
| net = Model() | ||
| net.eval() | ||
|
|
||
| torch.manual_seed(0) | ||
| x = torch.rand(1, 4, 8, 8) | ||
| y = torch.rand(1, 4, 8, 8) | ||
|
|
||
| a = net(x, y) | ||
|
|
||
| # 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: | ||
|
Comment on lines
+30
to
+52
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| pnnx_param = f.read() | ||
|
|
||
| if "pnnx.Expression" not in pnnx_param: | ||
| return False | ||
| if "e-40" not in pnnx_param: | ||
| return False | ||
|
|
||
| # ncnn inference | ||
| import test_ncnn_expression_denorm_ncnn | ||
|
|
||
| b = test_ncnn_expression_denorm_ncnn.test_inference() | ||
|
|
||
| for aa, bb in zip(a, b): | ||
| if not torch.allclose(aa, bb, 1e-6, 1e-6): | ||
| return False | ||
|
|
||
| return True | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| if test(): | ||
| exit(0) | ||
| else: | ||
| exit(1) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch. You're right that
strtofaccepts a valid prefix unlessendptris checked against the end of the string.One note: this is not a new behavior introduced by this PR. The previous code used
std::stofwithout checkingpos, which also accepts a valid prefix and ignores trailing characters.This PR is intentionally scoped to avoiding
std::out_of_rangeon denormal/underflow literals inpass_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.