Add support for s390x#1074
Open
AlekseiNikiforovIBM wants to merge 3 commits into
Open
Conversation
This change is needed to build onnxruntime-extension on s390x.
This change fixes following tests: test_blingfire_sentencebreaker.py::TestBlingFireSentenceBreaker::test_text_to_case1 test_blingfire_sentencebreaker.py::TestBlingFireSentenceBreaker::test_text_to_case2 test_blingfire_sentencebreaker.py::TestBlingFireSentenceBreaker::test_text_to_case3
This change fixes following tests: test_pp_api.py::TestPPAPI::test_qwen2_5_vl_image_processing test_pp_api.py::TestPPAPI::test_qwen3_vl_image_processing
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds endian-awareness (notably for big-endian/s390x) in image resampling and in vendored third-party dependencies by applying additional patch files during CMake FetchContent.
Changes:
- Make 32-bit pixel packing deterministic across endianness in
image_resample.c. - Apply an extra protobuf patch for s390x via CMake.
- Apply a large BlingFire patch adding little-endian decoding helpers and updating BlingFire’s packed-image readers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/api/image_resample.c | Introduces a LE packing macro to fix byte-order dependent writes. |
| cmake/externals/sentencepieceproject.cmake | Applies multiple protobuf patches during FetchContent. |
| cmake/externals/protobuf_s390x.patch | Adds an s390x-related protobuf portability tweak. |
| cmake/externals/blingfire_s390x.patch | Adds endian conversion utilities + updates BlingFire readers to decode LE-encoded data. |
| cmake/externals/blingfire.cmake | Applies multiple BlingFire patches during FetchContent. |
Comments suppressed due to low confidence (1)
cmake/externals/blingfire_s390x.patch:1
- Two issues in the newly introduced header content: (1) the include guard
_BF_ENDIAN_H_begins with an underscore followed by an uppercase letter, which is reserved in C/C++ and can technically cause undefined behavior or conflicts—rename to a non-reserved macro (e.g.,BF_ENDIAN_H_orBLINGFIRE_BF_ENDIAN_H_). (2) The same unguarded__BYTE_ORDER__/__ORDER_BIG_ENDIAN__check can mis-detect endianness (and can try to include<byteswap.h>on toolchains where it doesn’t exist). Adddefined(...)guards and a portable fallback strategy.
diff --git a/blingfireclient.library/inc/BFendian.h b/blingfireclient.library/inc/BFendian.h
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+42
to
+48
| #if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ | ||
| #define MAKE_UINT32_LE(u0, u1, u2, u3) \ | ||
| ((uint32_t)(u3) | ((uint32_t)(u2) << 8) | ((uint32_t)(u1) << 16) | ((uint32_t)(u0) << 24)) | ||
| #else // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ | ||
| #define MAKE_UINT32_LE(u0, u1, u2, u3) \ | ||
| ((uint32_t)(u0) | ((uint32_t)(u1) << 8) | ((uint32_t)(u2) << 16) | ((uint32_t)(u3) << 24)) | ||
|
|
||
| #endif // __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ |
| + | ||
| +#include <stdint.h> | ||
| + | ||
| +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ |
Comment on lines
+1
to
+3
| set(blingfire_patches | ||
| "${PROJECT_SOURCE_DIR}/cmake/externals/blingfire_cmake.patch" | ||
| "${PROJECT_SOURCE_DIR}/cmake/externals/blingfire_s390x.patch") |
| GIT_TAG 0831265c1aca95ca02eca5bf1155e4251e545328 | ||
| EXCLUDE_FROM_ALL | ||
| PATCH_COMMAND git checkout . && git apply --ignore-space-change --ignore-whitespace ${PROJECT_SOURCE_DIR}/cmake/externals/blingfire_cmake.patch) | ||
| PATCH_COMMAND git checkout . && git apply --ignore-space-change --ignore-whitespace ${blingfire_patches}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes fix build and runtime issues on s390x.
Patches are added to fix build issues in old protobuf version on s390x and to fix runtime issues in BlingFire dependency.
A couple of endianness issues in onnxruntime-extensions are fixed as well.