From 05adb3d8a3596a94e1620645bc194d1125cbd8de Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 05:34:01 +0000 Subject: [PATCH] Fix fuse_pad_into_pool folding zero-padding into MaxPool MaxPool ignores its padded elements, which is equivalent to padding with -inf. The pass previously folded a Pad with constant_value=0 (or an unspecified value, which defaults to 0) into MaxPool by moving the padding into the pool's `pads` attribute. This changes the result whenever a pooling window's real values are all negative: Pad(0)+MaxPool yields 0 while the fused MaxPool yields the (negative) window maximum. Make the required Pad constant value depend on the pool type: - AveragePool (with count_include_pad=1): 0 - MaxPool: -inf Also restructure the constant-value check so an unspecified Pad value correctly blocks fusion into MaxPool instead of being treated as a match. Update the MaxPool tests that encoded the old behavior to assert no fusion for value=0/default, and add companion tests confirming Pad(value=-inf) folds correctly. Fixes onnxsim/onnxsim#290 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_013sB8xKJyd86p47c2vfXMVD Signed-off-by: take-cheeze --- onnxoptimizer/passes/fuse_pad_into_pool.h | 78 +++++---- onnxoptimizer/test/optimizer_test.py | 192 +++++++++++++++++++++- 2 files changed, 236 insertions(+), 34 deletions(-) diff --git a/onnxoptimizer/passes/fuse_pad_into_pool.h b/onnxoptimizer/passes/fuse_pad_into_pool.h index 7be87de90..026d2a744 100644 --- a/onnxoptimizer/passes/fuse_pad_into_pool.h +++ b/onnxoptimizer/passes/fuse_pad_into_pool.h @@ -13,9 +13,15 @@ // After: // Z = pool(X, Y) with "pads" attribute set // -// the pass handles the case when Pad is zero-padding the input -// (i.e. mode=constant and constant_value=0) - +// The pass only fires when Pad uses mode=constant and the constant value +// matches the value the pool implicitly uses for its own padding: +// - AveragePool (with count_include_pad=1): constant_value = 0 +// - MaxPool: constant_value = -inf +// MaxPool ignores its padded elements, which is equivalent to padding with +// -inf. Folding a zero-padding Pad into a MaxPool is therefore incorrect +// (see https://github.com/onnxsim/onnxsim/issues/290). + +#include #include #include "onnx/defs/tensor_util.h" @@ -67,6 +73,17 @@ struct FusePadIntoPool final : public PredicateBasedPass { } // Process 'Constant_value' + // + // The Pad can only be fused when it fills the padded region with the same + // value the pool implicitly uses for its own padding: + // - AveragePool (with count_include_pad=1): 0 + // - MaxPool: -inf (MaxPool ignores padded elements, which is equivalent + // to padding with -inf). Folding a zero-padding Pad into a MaxPool + // would change the result whenever a window's real values are all + // negative -- see https://github.com/onnxsim/onnxsim/issues/290. + const bool is_maxpool = pool->kind() == Symbol("MaxPool"); + const double required_pad_value = + is_maxpool ? -std::numeric_limits::infinity() : double(0); { union ConstantValueType { int32_t i32; @@ -79,32 +96,35 @@ struct FusePadIntoPool final : public PredicateBasedPass { int16_t i16; } cv; -#define Define_GetConstantValueFromInput(token) \ - (GetValueFromInput(pad, 2, cv.token) && cv.token == decltype(cv.token)(0)) - - do { - if (GetValueFromAttr(pad, kvalue, cv.f64) && cv.f64 == double(0)) { - break; - } - if (pad->inputs().size() >= 3) { - if (pad->input(2)->uniqueName().empty()) { - break; - } - if (Define_GetConstantValueFromInput(i32) || - Define_GetConstantValueFromInput(i64) || - Define_GetConstantValueFromInput(f32) || - Define_GetConstantValueFromInput(f64) || - Define_GetConstantValueFromInput(ui8) || - Define_GetConstantValueFromInput(i8) || - Define_GetConstantValueFromInput(ui16) || - Define_GetConstantValueFromInput(i16)) { - break; - } - return false; - } - } while (0); - -#undef Define_GetConstantValueFromInput +#define Match_ConstantValueFromInput(token) \ + (GetValueFromInput(pad, 2, cv.token) && \ + static_cast(cv.token) == required_pad_value) + + bool pad_value_matches; + if (GetValueFromAttr(pad, kvalue, cv.f64)) { + // Explicit 'value' attribute (opset 10 and below). + pad_value_matches = (cv.f64 == required_pad_value); + } else if (pad->inputs().size() >= 3 && + !pad->input(2)->uniqueName().empty()) { + // Explicit 'constant_value' input (opset 11 and above). + pad_value_matches = Match_ConstantValueFromInput(i32) || + Match_ConstantValueFromInput(i64) || + Match_ConstantValueFromInput(f32) || + Match_ConstantValueFromInput(f64) || + Match_ConstantValueFromInput(ui8) || + Match_ConstantValueFromInput(i8) || + Match_ConstantValueFromInput(ui16) || + Match_ConstantValueFromInput(i16); + } else { + // No constant value specified: Pad defaults to 0. + pad_value_matches = (required_pad_value == double(0)); + } + +#undef Match_ConstantValueFromInput + + if (!pad_value_matches) { + return false; + } } // check if some values in 'pads' prevents us from fusing it into 'Conv' diff --git a/onnxoptimizer/test/optimizer_test.py b/onnxoptimizer/test/optimizer_test.py index 2ee7d3003..fc833b366 100644 --- a/onnxoptimizer/test/optimizer_test.py +++ b/onnxoptimizer/test/optimizer_test.py @@ -2081,6 +2081,9 @@ def test_fuse_pad_into_avgpool_no_optional_value_opset10(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [0, 0, 1, 1] def test_fuse_pad_into_maxpool_no_optional_value_opset10(self): + # A Pad with the default constant value (0) must NOT be folded into a + # MaxPool: MaxPool pads with -inf, so zero-padding changes the result. + # See https://github.com/onnxsim/onnxsim/issues/290 pad = helper.make_node( "Pad", ["X"], ["P"], mode="constant", pads=[0, 0, 0, 0, 0, 0, 1, 1] ) @@ -2098,6 +2101,33 @@ def test_fuse_pad_into_maxpool_no_optional_value_opset10(self): opset_imports=[helper.make_opsetid("", 10)], ) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_neg_inf_value_opset10(self): + # A Pad with constant value -inf is the correct fill value for MaxPool + # and can be folded into it. + pad = helper.make_node( + "Pad", + ["X"], + ["P"], + mode="constant", + pads=[0, 0, 0, 0, 0, 0, 1, 1], + value=float("-inf"), + ) + max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3, 3]) + graph = helper.make_graph( + [pad, max_pool], + "test", + [helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 2, 2))], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1, 1))], + ) + optimized_model = self._optimized( + graph, + ["fuse_pad_into_pool"], + False, + opset_imports=[helper.make_opsetid("", 10)], + ) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads" @@ -2133,6 +2163,9 @@ def test_fuse_pad_into_avgpool_no_optional_value(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [0, 0, 1, 1] def test_fuse_pad_into_maxpool_no_optional_value(self): + # No constant_value input => Pad defaults to 0, which must NOT be folded + # into MaxPool (MaxPool pads with -inf). + # See https://github.com/onnxsim/onnxsim/issues/290 pad = helper.make_node("Pad", ["X", "Pads"], ["P"], mode="constant") max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3, 3]) graph = helper.make_graph( @@ -2154,10 +2187,7 @@ def test_fuse_pad_into_maxpool_no_optional_value(self): ) optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) - assert len(list(optimized_model.graph.node)) == 1 - assert optimized_model.graph.node[0].op_type == "MaxPool" - assert optimized_model.graph.node[0].attribute[1].name == "pads" - assert list(optimized_model.graph.node[0].attribute[1].ints) == [0, 0, 1, 1] + assert optimized_model.graph == graph def test_fuse_pad_into_avgpool_with_optional_value(self): pad = helper.make_node("Pad", ["X", "Pads", "Constant_value"], ["P"], mode="constant") @@ -2196,7 +2226,10 @@ def test_fuse_pad_into_avgpool_with_optional_value(self): assert optimized_model.graph.node[0].attribute[2].name == "pads" assert list(optimized_model.graph.node[0].attribute[2].ints) == [0, 0, 1, 1] - def test_fuse_pad_into_maxpool_with_optional_value(self): + def test_fuse_pad_into_maxpool_with_zero_optional_value(self): + # This is the exact case reported in + # https://github.com/onnxsim/onnxsim/issues/290 : a Pad with an explicit + # constant value of 0 must NOT be folded into a MaxPool. pad = helper.make_node("Pad", ["X", "Pads", "Constant_value"], ["P"], mode="constant") max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3, 3]) graph = helper.make_graph( @@ -2225,6 +2258,39 @@ def test_fuse_pad_into_maxpool_with_optional_value(self): ) optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_with_neg_inf_optional_value(self): + # A Pad with constant value -inf is the correct fill value for MaxPool + # and can be folded into it. + pad = helper.make_node("Pad", ["X", "Pads", "Constant_value"], ["P"], mode="constant") + max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3, 3]) + graph = helper.make_graph( + [pad, max_pool], + "test", + [ + helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 2, 2)), + ], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1, 1))], + [ + helper.make_tensor( + "Pads", + TensorProto.INT64, + dims=(8,), + vals=np.array([0, 0, 0, 0, 0, 0, 1, 1]).astype(np.int64).tobytes(), + raw=True, + ), + helper.make_tensor( + "Constant_value", + TensorProto.FLOAT, + dims=(), + vals=np.array([float("-inf")]).astype(np.float32).tobytes(), + raw=True, + ), + ], + ) + optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads" @@ -2317,6 +2383,7 @@ def test_fuse_pad_into_avgpool_1d_opset10(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [1, 1] def test_fuse_pad_into_maxpool_1d_opset10(self): + # Default constant value (0) => must NOT fold into MaxPool. pad = helper.make_node("Pad", ["X"], ["P"], mode="constant", pads=[0, 0, 1, 0, 0, 1]) max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3]) graph = helper.make_graph( @@ -2332,6 +2399,27 @@ def test_fuse_pad_into_maxpool_1d_opset10(self): opset_imports=[helper.make_opsetid("", 10)], ) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_1d_neg_inf_opset10(self): + pad = helper.make_node( + "Pad", ["X"], ["P"], mode="constant", pads=[0, 0, 1, 0, 0, 1], + value=float("-inf"), + ) + max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3]) + graph = helper.make_graph( + [pad, max_pool], + "test", + [helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 1))], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1))], + ) + optimized_model = self._optimized( + graph, + ["fuse_pad_into_pool"], + False, + opset_imports=[helper.make_opsetid("", 10)], + ) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads" @@ -2367,6 +2455,7 @@ def test_fuse_pad_into_avgpool_1d(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [1, 1] def test_fuse_pad_into_maxpool_1d(self): + # No constant_value input => Pad defaults to 0 => must NOT fold. pad = helper.make_node("Pad", ["X", "Pads"], ["P"], mode="constant") max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3]) graph = helper.make_graph( @@ -2388,6 +2477,39 @@ def test_fuse_pad_into_maxpool_1d(self): ) optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_1d_neg_inf(self): + pad = helper.make_node( + "Pad", ["X", "Pads", "Constant_value"], ["P"], mode="constant" + ) + max_pool = helper.make_node("MaxPool", ["P"], ["Z"], kernel_shape=[3]) + graph = helper.make_graph( + [pad, max_pool], + "test", + [ + helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 1)), + ], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1))], + [ + helper.make_tensor( + "Pads", + TensorProto.INT64, + dims=(6,), + vals=np.array([0, 0, 1, 0, 0, 1]).astype(np.int64).tobytes(), + raw=True, + ), + helper.make_tensor( + "Constant_value", + TensorProto.FLOAT, + dims=(), + vals=np.array([float("-inf")]).astype(np.float32).tobytes(), + raw=True, + ), + ], + ) + optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads" @@ -2424,6 +2546,7 @@ def test_fuse_pad_into_avgpool_existing_avgpool_pad_opset10(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [1, 1, 1, 1] def test_fuse_pad_into_maxpool_existing_maxpool_pad_opset10(self): + # Default constant value (0) => must NOT fold into MaxPool. pad = helper.make_node( "Pad", ["X"], ["P"], mode="constant", pads=[0, 0, 0, 0, 0, 0, 1, 1] ) @@ -2443,6 +2566,29 @@ def test_fuse_pad_into_maxpool_existing_maxpool_pad_opset10(self): opset_imports=[helper.make_opsetid("", 10)], ) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_existing_maxpool_pad_neg_inf_opset10(self): + pad = helper.make_node( + "Pad", ["X"], ["P"], mode="constant", pads=[0, 0, 0, 0, 0, 0, 1, 1], + value=float("-inf"), + ) + max_pool = helper.make_node( + "MaxPool", ["P"], ["Z"], kernel_shape=[3, 3], pads=[1, 1, 0, 0] + ) + graph = helper.make_graph( + [pad, max_pool], + "test", + [helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 1, 1))], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1, 1))], + ) + optimized_model = self._optimized( + graph, + ["fuse_pad_into_pool"], + False, + opset_imports=[helper.make_opsetid("", 10)], + ) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads" @@ -2483,6 +2629,7 @@ def test_fuse_pad_into_avgpool_existing_avgpool_pad(self): assert list(optimized_model.graph.node[0].attribute[2].ints) == [1, 1, 1, 1] def test_fuse_pad_into_maxpool_existing_maxpool_pad(self): + # No constant_value input => Pad defaults to 0 => must NOT fold. pad = helper.make_node("Pad", ["X", "Pads"], ["P"], mode="constant") max_pool = helper.make_node( "MaxPool", ["P"], ["Z"], kernel_shape=[3, 3], pads=[1, 1, 0, 0] @@ -2506,6 +2653,41 @@ def test_fuse_pad_into_maxpool_existing_maxpool_pad(self): ) optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert optimized_model.graph == graph + + def test_fuse_pad_into_maxpool_existing_maxpool_pad_neg_inf(self): + pad = helper.make_node( + "Pad", ["X", "Pads", "Constant_value"], ["P"], mode="constant" + ) + max_pool = helper.make_node( + "MaxPool", ["P"], ["Z"], kernel_shape=[3, 3], pads=[1, 1, 0, 0] + ) + graph = helper.make_graph( + [pad, max_pool], + "test", + [ + helper.make_tensor_value_info("X", TensorProto.FLOAT, (1, 5, 1, 1)), + ], + [helper.make_tensor_value_info("Z", TensorProto.FLOAT, (1, 5, 1, 1))], + [ + helper.make_tensor( + "Pads", + TensorProto.INT64, + dims=(8,), + vals=np.array([0, 0, 0, 0, 0, 0, 1, 1]).astype(np.int64).tobytes(), + raw=True, + ), + helper.make_tensor( + "Constant_value", + TensorProto.FLOAT, + dims=(), + vals=np.array([float("-inf")]).astype(np.float32).tobytes(), + raw=True, + ), + ], + ) + optimized_model = self._optimized(graph, ["fuse_pad_into_pool"]) + assert len(list(optimized_model.graph.node)) == 1 assert optimized_model.graph.node[0].op_type == "MaxPool" assert optimized_model.graph.node[0].attribute[1].name == "pads"