[Christian Bourjau (cbourjau)]

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.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 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.txt to 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 Sign and Clip on 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) (or a_min=None, a_max=0) avoids relying on min/max keyword 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.

[Copilot]

🟡 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

[Christian Bourjau (cbourjau)]

I moved the tests to c++

[Copilot]

🟡 Changes recommended

The new Clip int64 regression test likely needs the existing DirectML skip guard (as used by Clip_Default_int64) to avoid known DML failures for int64 Clip until #41968513 is resolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Christian Bourjau (cbourjau)]

Hariharan Seshadri (@hariharans29) Ti-Tai Wang (@titaiwangms), maybe you could take a look since you touched (or reviewed changes to) deps.txt not too long ago?

[mirounga]

The vcpkg port was not updated: cmake/vcpkg-ports/eigen3/portfile.cmake:10 still pins the old commit with no pcmp patch, and Windows CI builds with --use_vcpkg, so those builds keep the buggy SSE2 compare and the new tests would fail there.
The failure pattern fits: Linux arm64 and most macOS jobs pass while x64/x86 test jobs fail.
DML: Copilot's open comment says the new test needs the same DML skip as Clip_Default_int64; the DML pipeline failure supports that.
What the thread pool needs from Eigen: Eigen::TensorCostModel / TensorOpCost with kStartupCycles = kPerThreadCycles = 100000 and kTaskSize = 40000. Ask for a TensorCostModel.h diff and a one-session-per-process perf check — #32459's new test hard-codes kStartupCycles.

[Christian Bourjau (cbourjau)]

mirounga Thanks for taking a look. Here are the latest updates.

• vcpkg port: updated cmake/vcpkg-ports/eigen3 to the same 5.0.1 commit as deps.txt (bc3b3987). It now applies the same patches as the FetchContent path, including the SSE2 pcmp_lt
  backport. I dropped the three port patches that were never applied, and the Eigen3Config.cmake rewrite, since 5.0.1 already guards on Eigen3::Eigen.
• DML: Clip_int64_above_int32_max now has the same DML skip as Clip_Default_int64.
• Thread-pool cost model: I don't think anything changed for the cost models. As can (not) be seen with Ctrl-F for costmodel here: https://gitlab.com/libeigen/eigen/-/compare/1d8b82b0740839c0de7f1242a3585e3390ff5f33...bc3b39870ecb690a623a3f49149a358b95c5781d?straight=true (i.e. diff from the currently used commit to 5.0.1)

[Copilot]

🔵 Needs a closer look

The shared numerical dependency upgrade needs human review of cross-platform build and test results for both dependency paths.

0 open findings

1 resolved since last review

• This new int64 Clip regression test doesn’t include the existing DirectML (DML) skip that’s already…

🧠 Review effort: Balanced

[Christian Bourjau (cbourjau)]

The QNN test failure is expected since QNN narrows int64 to int32, which produces wrong results for the test in question. I excluded QNN for that test. The other failures are due to the new eigen commit not being available on the vcpkg mirror. Changming Sun (@snnn) , could you maybe take a look?

[Changming Sun (snnn)]

Thanks, Christian. I have left Microsoft. Someone else will help review this PR.

[Christian Bourjau (cbourjau)]

mirounga eserscor do you think you might be able to review this?

[Christian Bourjau (cbourjau)]

Thanks eserscor ! I think the remaining issues should be fixed with the latest changes. Could you trigger the CI again, please?

[Christian Bourjau (cbourjau)]

eserscor, I'm afraid I don't have permission to merge. Would be nice to have this in the bag for the next release.

[Christian Bourjau (cbourjau)]

Sorry for the spam! I hope you don't mind me pinging this again. Would be great to have this in the bag for the next release.

eserscor Ti-Tai Wang (@titaiwangms) mirounga Hariharan Seshadri (@hariharans29)

[eserscor]

Apologies I missed the spam, merged.

[Christian Bourjau (cbourjau)]

I'm afraid we missed the rel-1.32.0 branch cut by a few hours. Do you think we could cherry-pick this PR into it?