-
Notifications
You must be signed in to change notification settings - Fork 1.1k
cpu: add loongarch64 support #1923
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
Conversation
| - [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
|
|
||
| #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 supportshould 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=loongarch64cmake option). Do you have any plan to add loongarch64 isa specific code to oneDNN?
| # 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! |
ranukund
left a comment
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.
Looks good, thanks!
|
Closing as stale. |
about loongarch:https://docs.kernel.org/arch/loongarch/introduction.html