-
Notifications
You must be signed in to change notification settings - Fork 981
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
cpu: add loongarch64 support #1923
base: main
Are you sure you want to change the base?
Conversation
@@ -74,10 +74,10 @@ oneDNN supports platforms based on the following architectures: | |||
- [OpenPOWER](https://openpowerfoundation.org/) / [IBM Power ISA](https://en.wikipedia.org/wiki/Power_ISA). | |||
- [IBMz z/Architecture (s390x)](https://en.wikipedia.org/wiki/Z/Architecture). | |||
- [RISC-V 64-bit (RV64)](https://en.wikipedia.org/wiki/RISC-V). | |||
|
|||
- [LOONGARCH 64 bit](https://docs.kernel.org/arch/loongarch/introduction.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please restore the new line to separate the warning.
src/cpu/platform.hpp
Outdated
// - DNNL_ARCH_GENERIC | ||
// Target architecture macro is set to 1, others to 0. All macros are defined. | ||
|
||
#if defined(DNNL_X64) + defined(DNNL_AARCH64) + defined(DNNL_PPC64) \ | ||
+ defined(DNNL_S390X) + defined(DNNL_RV64) \ | ||
+ defined(DNNL_S390X) + defined(DNNL_RV64) + defined(DNNL_LOONGARCH64) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply clang-format on this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply clang-format on this file.
It's been revised and resubmitted, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR. I have few questions:
- could you reword the commit message to align with oneDNN guidelines (see here. Something like
cpu: add loongarch64 support
should do. - In general, adding a new CPU to oneDNN platform.cmake is necessary only when isa specific optimization or fixes are added to the library. From the look of the patch, it does not seem to be the case for loongarch64 (this patch is equivalent to building the library with
ONEDNN_ARCH_OPT_FLAGS=-march=loongarch64
cmake option). Do you have any plan to add loongarch64 isa specific code to oneDNN?
@@ -363,6 +363,8 @@ elseif(UNIX OR MINGW) | |||
elseif(DNNL_TARGET_ARCH STREQUAL "RV64") | |||
# G = General-purpose extensions, C = Compression extension (very common). | |||
append(DEF_ARCH_OPT_FLAGS "-march=rv64gc") | |||
elseif(DNNL_TARGET_ARCH STREQUAL "LOONGARCH64") | |||
append(DEF_ARCH_OPT_FLAGS "-march=loongarch64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not add opt flags for compilers other than gcc ?
Hello, I have changed the commit as requested.
Because I will add loongarch related code later, I didn't change it this time because of time constraints. |
@loong-hy do you mind submitting this patch as part of the future PR that will add loongarch specific code? |
Sure, I'll revise and resubmit the code shortly, thanks! |
about loongarch:https://docs.kernel.org/arch/loongarch/introduction.html