Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic RISC-V Support #18115

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Basic RISC-V Support #18115

wants to merge 3 commits into from

Conversation

CNOCycle
Copy link

This PR is the proposal to contribute RISC-V support mentioned in #17466. The focus here is on the basic RISC-V ISA (rv64imafdc), excluding the V or other extensions.

This PR comprises two parts. Firstly, we explicitly set the atomic linking flag and include some RISC-V related arguments in the build scripts. Secondly, we implement a scalar MLAS for RISC-V by leveraging the existing implementation in the folder onnxruntime/core/mlas/lib/scalar/.

The benefit of this solution is that we can follow the build logic defined in cmake/onnxruntime_mlas.cmake. As shown in below, if CMAKE_SYSTEM_PROCESSOR is an unsupported architecture, the scalar kernels are compiled. However, in onnxruntime/core/mlas/lib/sgemm.cpp, the default function MlasSgemmCopyPackB assumes that the data can be packed with SIMD instructions, where data are encapsulated in type MlasLoadFloat32x4, which is incompatible with the scalar implementation. Therefore, we need an additional macro defined in onnxruntime/core/mlas/inc/mlas.h to ensure that MlasSgemmCopyPackB is compatible with the scalar version. Notably, we do not require the complex modifications proposed by #16768.

diff --git a/cmake/onnxruntime_mlas.cmake b/cmake/onnxruntime_mlas.cmake
index 6828dfd076..a5d6c28e8b 100644
--- a/cmake/onnxruntime_mlas.cmake
+++ b/cmake/onnxruntime_mlas.cmake
@@ -571,7 +571,7 @@ else()
     endif()
-     if(NOT ONNXRUNTIME_MLAS_MULTI_ARCH AND MLAS_SOURCE_IS_NOT_SET)
+     if(NOT ONNXRUNTIME_MLAS_MULTI_ARCH AND MLAS_SOURCE_IS_NOT_SET) # Highlight
         file(GLOB_RECURSE mlas_platform_srcs
          "${MLAS_SRC_DIR}/scalar/*.cpp")
     endif()
     target_sources(onnxruntime_mlas PRIVATE ${mlas_platform_srcs})
 endif()

We conducted two CI tests (./build.sh --test --config Release --allow_running_as_root and onnxruntime_mlas_test) and encountered only one failure related to numerical compatibility.

This issue is caused by the inconsistent behavior of NaN propagation as delineated in the IEEE 754-2019 standard [2] and the RISC-V specification [3].

Specifally, the intricacies of NaN propagation are comprehensively outlined in chapter 6.2.3 of [2] and chapter 11.3 of [3]. The IEEE standard stipulates that "An operation that propagates a NaN operand to its result and has a single NaN as an input should produce a NaN with the payload of the input NaN if it's representable in the destination format." In contrast, the RISC-V specification defines that "Except when otherwise stated, if the result of a floating-point operation is NaN, it is the canonical NaN. The canonical NaN has a positive sign and all significand bits clear except the MSB, also known as the quiet bit. For single-precision floating-point, this corresponds to the pattern 0x7fc00000."

In summary, while the IEEE standard retains the remaining bits when propagating NaN, RISC-V converts NaNs to 0x7fc00000.

I am receptive to feedback, collaboration, and further guidance to effectively contribute to supporting the RISC-V architecture.

References

[1] https://github.com/ucb-bar/onnxruntime-riscv

[2] Microprocessor Standards Committee. (2019). 754-2019-IEEE Standard for Floating-Point Arithmetic.

[3] The RISC-V Instruction Set Manual Volume I: Unprivileged ISA. (2019). https://riscv.org/wp-content/uploads/2019/06/riscv-spec.pdf

Some stable RISC-V toolchains don't support inline subword atomic
operations, so we explicitly link libatomic.

Reviewed-by: Alan Kao <alankao@andestech.com>
Reviewed-by: Alan Kao <alankao@andestech.com>
As a baseline patch, this preliminary RISC-V port leverages the
existing scalar implementation that was previously only utilized by
WASM target.

Also, we rename TARGET_WASM_SCALAR macros to TARGET_SCALAR, which
serves as a neutral designation that accommodates the inclusion
of RISC-V as a scalar target.

Reviewed-by: Alan Kao <alankao@andestech.com>
@CNOCycle CNOCycle requested a review from a team as a code owner October 26, 2023 15:50
@CNOCycle
Copy link
Author

@microsoft-github-policy-service agree

@CNOCycle
Copy link
Author

CNOCycle commented Dec 5, 2023

We developed a standalone script for building the onnxruntime package on RISC-V architecture. The script leverages user-space QEMU within a Docker container, ensuring seamless integration with existing CI systems. In addition to this, our team is enthusiastic about contributing to the community by offering support in maintenance, issue resolution, and fostering future growth. We remain eager to move ahead with the discussion on RISC-V support and the nuanced topic of inconsistent behavior in NaN propagation, as previously mentioned.

If the community express continued interest in RISC-V support, we're more than willing to provide a comprehensive guideline to facilitate its implementation.

Looking forward to further collaboration and progress.

@justinchuby
Copy link
Contributor

cc @faxu

@snnn
Copy link
Member

snnn commented Jan 5, 2024

And #19019 is also related to this.

@yihonglyu
Copy link
Contributor

@CNOCycle Could you resolve the conflicts?

@CNOCycle
Copy link
Author

I am pleased to hear about the progrsses since the submission of this PR. Before we address the conflicts, I believe it would be valuable to gather feedback on the PR, as there are at least three other proposed implementations (#16768, #19238) and the consideration of implementing the RISC-V provider as an alternative solution.

The primary objective of this PR is to extend support for onnxruntime on the RISC-V architecture while minimizing modifications and unintentional behavioral changes on existing architectures. As highlighted in the PR description, modifications have been made to the codes in the MLAS component. I would appreciate your insights on whether these modifications align with your expectations and are deemed acceptable.

In addition to our standalone solution including a docker-based CI pipeline has implemented in our internal system, however, PR #19238 has recently been merged. I recommend maintainers ask whether they encounter any related issues for their solution. If no such issues arise, it might be worth considering to drop this PR and our following implementations.

Your feedback on these matters is highly valued, and we are open to any suggestions or modifications that align with the project's goals. Thank you for your time and consideration.

@FarbodNajafi11
Copy link

FarbodNajafi11 commented Feb 13, 2024

Does anyone know how to program a menu on LCD screen using RISC V ?

@csukuangfj
Copy link
Contributor

Any update on this PR?

@CNOCycle
Copy link
Author

CNOCycle commented Mar 8, 2024

Unfortunately, the official attitude is ambiguous and we have not received any feedback from them on this PR. However, if you are interested in onnxruntime on pure RISC-V CPU support instead of xnnpack provider, you can involve the proposed modifications in your local branch. Our support for the latest version is currently suspended, but it is compatible with v14.0, v15.1, v16.0.

@lamaalrajih
Copy link

lamaalrajih commented Sep 23, 2024

@snnn @tianleiwu could we get onnxruntime support on riscv?

@tianleiwu
Copy link
Contributor

tianleiwu commented Sep 23, 2024

@CNOCycle, The minimum requirement is that all build pipelines are green. For example, one required pipeline (Lint/Python format) failed. You will need run lintrunner to fix the format like

pip3 install -r requirements-lintrunner.txt
lintrunner init
lintrunner -a

See https://github.com/microsoft/onnxruntime/blob/main/docs/Coding_Conventions_and_Standards.md#linting for more info.

Once that is passed, we can run more pipelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants