Add extended support from new Neutron C flow for Clamp operator#19510
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19510
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 54e327d with merge base b04cc65 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
3a90b03 to
f65fe45
Compare
There was a problem hiding this comment.
Pull request overview
This PR moves the use_new_flow_neutron_c flag from CustomDelegationOptions onto NeutronTargetSpec so it can be threaded through the quantizer and converters as part of target configuration, and extends the new Neutron‑C flow's clamp support beyond Relu‑shaped bounds by emitting Maximum/Minimum ops with quantized constants.
Changes:
- Move
use_new_flow_neutron_cfromCustomDelegationOptionstoNeutronTargetSpec, updating all call sites and converters that read the flag. - Extend
ClampConverterto lower arbitrary clamp bounds to aMax+Minsequence under the new flow, factoring_is_convertible_to_reluout as a helper used by both the converter and the newClampPattern.get_anchors. - Refactor
QuantizationPatternto requireneutron_quantizerin the base constructor, propagate this through all subclasses, and reworkClampPatternto use a shared‑spec anchor under the new flow.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backends/nxp/backend/custom_delegation_options.py | Removes use_new_flow_neutron_c from CustomDelegationOptions. |
| backends/nxp/backend/neutron_target_spec.py | Adds use_new_flow_neutron_c constructor argument and attribute. |
| backends/nxp/nxp_backend.py | Threads flag into NeutronTargetSpec constructions; drops it from CustomDelegationOptions. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/abs_converter.py | Reads new flag from target spec instead of delegation options. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/avg_pool_2d_converter.py | Same flag-source switch. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/max_pool2d_with_indices_converter.py | Same flag-source switch. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/mul_tensor_converter.py | Same flag-source switch. |
| backends/nxp/backend/ir/converter/node_converters/ops_converters/clamp_converter.py | Adds Max/Min lowering for non‑Relu clamp bounds under new flow; introduces _is_convertible_to_relu. |
| backends/nxp/quantizer/patterns.py | QuantizationPattern now takes neutron_quantizer; ClampPattern adds shared‑spec anchors when new flow is on. |
| backends/nxp/quantizer/neutron_quantizer.py | Passes self into all pattern constructors. |
| backends/nxp/tests/executorch_pipeline.py | Construct NeutronTargetSpec with the new flag; minor formatting. |
| backends/nxp/tests/ir/converter/node_converter/test_clamp_converter.py | Adds parameterized tests for clamp under the new Neutron‑C flow. |
| examples/nxp/aot_neutron_compile.py | Forwards CLI flag into NeutronTargetSpec. |
Comments suppressed due to low confidence (1)
backends/nxp/backend/ir/converter/node_converters/ops_converters/clamp_converter.py:169
- The min/max constant tensors are hard-coded to
np.int8, but_is_supported_on_targetreturnsTrueunconditionally whenuse_new_flow_neutron_cis set, with no dtype filter. If the input isuint8(which is listed as a supported type by other converters in this flow, e.g.abs_converter.py,avg_pool_2d_converter.py), values >127 will silently overflow when stored asint8, producing incorrect outputs. The dtype passed tonp.arrayshould be derived from the dequant node's dtype (or fromquant_min/quant_max).
min_tensor = self.builder.create_tensor_for_data(
np.array([min_value], np.int8), "min"
)
self.propagate_quantization(x, min_tensor)
max_tensor = self.builder.create_tensor_for_data(
np.array([max_value], np.int8), "max"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Make sure the `clamp` was NOT delegated. | ||
| assert graph_contains_any_of_ops(delegated_ep.graph, [Clamp]) | ||
|
|
There was a problem hiding this comment.
Please add the new tests inside a class. It's a new convention we have started with the new flow:
class TestClampNewNeutronFlow:
We can then use shorter test names with improved clarity.
Please draw inspiration from other open PRs which enable new ops (e.g. here). The TestClampNewNeutronFlow class should test all aspects of the operator with the new flow, because eventually we will remove the old tests with the old flow. So please add more tests.
There was a problem hiding this comment.
The tests are now inside a class. Also, added a broader set of test cases + more robust testing including verification of producing ReLU vs. Min/Max. I had to resort to spying on TFlite builder itself. Can you have a look?
| pytest.param(10, 17, id="min = 10, max = 17 (Max/Min)"), | ||
| pytest.param(0, 1, id="min = 0, max = 1 (Relu0To1)"), | ||
| pytest.param(-1, 1, id="min = -1, max = 1 (ReluN1To1)"), | ||
| pytest.param(0, None, id="min = 0, max = None (Relu)"), |
There was a problem hiding this comment.
Would be nice to see some assertions that the node is indeed converted to a Relu variant when possible.
There was a problem hiding this comment.
Added checking in TFLite. (See my response in your other comment.)
roman-janik-nxp
left a comment
There was a problem hiding this comment.
Needs a rework.
f65fe45 to
f3c4576
Compare
f3c4576 to
2708c0c
Compare
|
|
Here we go again with Copilot... 😄 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
backends/nxp/tests/ir/converter/node_converter/test_clamp_converter.py:37
exir_opsis imported but not used anywhere in this test module, which will fail unused-import linting (F401). Remove this import to keep the test file lint-clean.
)
from executorch.exir.dialects._ops import ops as exir_ops
from executorch.backends.nxp.tests.use_qat import *
backends/nxp/backend/ir/converter/node_converters/ops_converters/clamp_converter.py:210
max_tensoris created with a hard-codednp.int8dtype. If the clamp IO quantization dtype isuint8, this will produce an incorrect constant tensor type/value. Create the constant using the same dtype as the clamp input/output quantization.
if max_value is not None:
max_value = self._quantize_value(max_value, zp, scale, quant_min, quant_max)
max_tensor = self.builder.create_tensor_for_data(
np.array([max_value], np.int8), "max"
)
2708c0c to
3ddb14b
Compare
| dequant_node = list(intermediate_ep.graph.nodes)[-4] | ||
| tflite_internal_ops = list( | ||
| op.builtin_code for op in tflite_spy.spy_return.operator_codes.vector | ||
| ) |
3ddb14b to
7b06dec
Compare
7b06dec to
54e327d
Compare
Summary
CustomCompileConfigtoNeutronTargetSpecTest plan
New test cases for Clamp are introduced. The relocation of new flag is covered by already existing unit tests.
cc @robert-kalmar @JakeStevens @digantdesai @rascani