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

loongarch: add loongarch support #225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HecaiYuan
Copy link

hi! @adamjw24 This patch provides support for Loongarch.

CMakeLists.txt Outdated
message( STATUS "LoongArch64 lasx intrinsics disabled" )
endif()
endif()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please only add the -mlasx -mlsx flags to the SSE41 SIMD files (same way as -mavx2 is handled). This can be done in source/Lib/vvdec/CMakeLists.txt

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the modifications.

Copy link
Member

@adamjw24 adamjw24 Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to go back on it, but a colleagues had a good comment on this. Does longaarch64 mandate LASX and LSX? If so, yoru first solution was actually the better option. Could you let me know, and if it does mandate the vector extension, revert back to the first option? Thanks for the patch and sorry for the confusion

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When compiling 128-bit simd code, you need to add mlsx, and when compiling 256-bit simd code, you need to add mlasx. Based on the first solution, the relevant information regarding lasx has been removed.

@HecaiYuan HecaiYuan force-pushed the master branch 2 times, most recently from 24e099f to efef492 Compare January 8, 2025 02:08
CMakeLists.txt Outdated
if( VVDEC_TARGET_ARCH STREQUAL "LOONGARCH64" )
if( VVDEC_ENABLE_LOONGARCH64_LSX_SIMD )
set_if_compiler_supports_flag( FLAG_mlsx -mlsx )
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -mlsx")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use the variable FLAG_mlsx? As in:

      set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${FLAG_mlsx}")
      set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLAG_mlsx}")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that looks good to me. @K-os what do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse me. I made a mistake here. It should be ${FLAG_mlsx}. @adamjw24

@adamjw24 adamjw24 requested a review from K-os January 8, 2025 12:51
Copy link
Collaborator

@K-os K-os left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good to me.

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.

3 participants