Update Eigen to 5.0.1 + SSE2 fix (broken Clip & Sign) - #32475
Update Eigen to 5.0.1 + SSE2 fix (broken Clip & Sign)#32475Christian Bourjau (cbourjau) wants to merge 2 commits into
Conversation
Signed-off-by: Christian Bourjau <christian.bourjau@quantco.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The newly added Python regression tests are currently model-invalid/fragile (domain/opset mismatch, Constant attribute usage, NumPy clip call) and may not exercise the CPU/Eigen path unless CPUExecutionProvider is forced.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the pinned Eigen dependency to the 5.0.1 release and introduces a new Eigen patch to fix an SSE2 int64 compare bug that can break CPU Sign and Clip behavior for specific values (e.g., 2147483649).
Changes:
- Bump Eigen source pin in
cmake/deps.txtto a 5.0.1 commit archive/hash. - Update Eigen patch application to drop the old s390x build patch and add a new SSE2 int64-compare backport patch.
- Add Python regression tests for
SignandClipon int64 inputs.
File summaries
| File | Description |
|---|---|
| onnxruntime/test/python/test_sign.py | Adds a minimal ONNX model + regression test for Sign with problematic int64 values. |
| onnxruntime/test/python/test_clip.py | Adds a minimal ONNX model + regression test for Clip with problematic int64 values. |
| cmake/patches/eigen/sse2-pcmp-lt-int64-sign.patch | New Eigen backport patch to fix SSE2 Packet2l less-than compare behavior. |
| cmake/patches/eigen/s390x-build.patch | Removes an older s390x-focused patch that is no longer applied. |
| cmake/external/eigen.cmake | Adjusts which Eigen patches are applied during FetchContent. |
| cmake/deps.txt | Updates Eigen URL/hash and refreshes the pinning commentary. |
Review details
Suppressed comments (3)
onnxruntime/test/python/test_sign.py:30
- This test is intended to validate the CPU/Eigen implementation, but InferenceSession without an explicit providers list may run the model on CUDA/other EPs when available and miss the regression. Force CPUExecutionProvider to ensure the SSE2 code path is exercised on capable machines.
sess = ort.InferenceSession(make_sign_model().SerializeToString())
x = np.asarray([2147483649, 2147483649], np.int64)
(res,) = sess.run(None, {"x": x})
onnxruntime/test/python/test_clip.py:15
- The Clip node is set to domain="ai.onnx" but the model imports the default opset domain (""). To keep the model unambiguous and consistent with other tests, omit the domain field for standard ONNX ops.
clip_node = helper.make_node(
op_type="Clip",
domain="ai.onnx",
inputs=["x", "", "max"],
outputs=["y"],
)
onnxruntime/test/python/test_clip.py:33
- Use np.clip’s standard argument names/ordering for portability across NumPy versions.
np.clip(x, None, 0)(ora_min=None, a_max=0) avoids relying onmin/maxkeyword aliases.
np.testing.assert_array_equal(res, np.clip(x, min=None, max=0))
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def make_clip_model() -> onnx.ModelProto: | ||
| max_node = helper.make_node(op_type="Constant", domain="ai.onnx", inputs=[], outputs=["max"], value_int=0) | ||
| clip_node = helper.make_node( |
| node = helper.make_node( | ||
| op_type="Sign", | ||
| domain="ai.onnx", | ||
| inputs=["x"], | ||
| outputs=["y"], | ||
| ) |
| sess = ort.InferenceSession(make_clip_model().SerializeToString()) | ||
| x = np.asarray([2147483649, 2147483649], np.int64) | ||
| (res,) = sess.run(None, {"x": x}) |
There was a problem hiding this comment.
🟡 Changes recommended
The new Python regression tests should be aligned with the repo’s test conventions (headers and unittest-style discovery) to ensure they reliably execute in CI.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
onnxruntime/test/python/test_clip.py:36
- Tests in this folder are typically implemented as unittest.TestCase methods (see test_pep561_py_typed_marker.py), which works with both unittest discovery and pytest runners. As a free function, this test may be skipped depending on how the suite is executed.
def test_clip():
sess = ort.InferenceSession(make_clip_model().SerializeToString(), providers=["CPUExecutionProvider"])
x = np.asarray([2147483649, 2147483649], np.int64)
(res,) = sess.run(None, {"x": x})
onnxruntime/test/python/test_sign.py:30
- Tests in this folder are typically implemented as unittest.TestCase methods (see test_pep561_py_typed_marker.py), which works with both unittest discovery and pytest runners. As a free function, this test may be skipped depending on how the suite is executed.
def test_sign():
sess = ort.InferenceSession(make_sign_model().SerializeToString(), providers=["CPUExecutionProvider"])
x = np.asarray([2147483649, 2147483649], np.int64)
(res,) = sess.run(None, {"x": x})
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
| import numpy as np | ||
| import onnx | ||
| from onnx import TensorProto, helper | ||
|
|
||
| import onnxruntime as ort |
| import numpy as np | ||
| import onnx | ||
| from onnx import TensorProto, helper | ||
|
|
||
| import onnxruntime as ort |
Description
onnxruntime is currently using an Eigen commit from somewhere in between Eigen 3.4.1 and Eigen 5.0.1 (despite the comment in deps.txt). This PR updates Eigen to the latest 5.0.1 release. Note that Eigen is a headers-only library, which should make this process smooth.
One of the currently applied patches is no longer needed (it is part of the 5.0.1 release), and the other is simply disabling some warnings (still present after this PR). The new patch backports an upstream fix for a bug which I had attempted to fix here somewhat incorrectly a long time ago. My proposed fix merely avoided the broken SSE2 code path.
Motivation and Context
The main motivation is to properly fix the Sign and Clip operators on installations that use the SSE2 path. Onnxruntime is currently using an outdated and odd Eigen version, which makes backporting awkward. It seems much easier and generally beneficial to update Eigen to a proper release, plus some well-defined patches.