-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[libpng] fix mips64 support #26265
[libpng] fix mips64 support #26265
Conversation
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.
Don't necessarily have a problem with this but need question answered.
add_definitions(-DPNG_MIPS_MSA_OPT=2) | ||
+ if("${CMAKE_C_COMPILER_VERSION}" VERSION_GREATER_EQUAL "7") | ||
+ add_definitions(-D__mips_msa) | ||
+ add_compile_options(-mmsa -mhard-float -mfp64) |
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.
Why isn't this coming from the toolchain? I'm nervous about embedding assumptions about the target like this...
Ping @Jamlys for response. |
60f017c
if(${PNG_MIPS_MSA} STREQUAL "on") | ||
add_definitions(-DPNG_MIPS_MSA_OPT=2) | ||
+ else() | ||
+ add_definitions(-DPNG_MIPS_MSA_CHECK_SUPPORTED) |
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.
Can you show that this has been submitted upstream and/or documentation from upstream that this is the correct thing to do?
(It looks correct to me but I'm very nervous about 'putting words in upstream's mouth')
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.
(To be clear, I'm not saying 'it has to be landed by upstream before we will land this', I'm saying that I think they have a right-of-first-refusal; as long as they have been notified and indicated agreement with the general direction, or have had some reasonable amount of time to respond, that's good enough)
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.
libpng has a simple specification about support for mips msa detection.
https://github.com/glennrp/libpng/tree/a37d4836519517bdce6cb9d956092321eca3e73b/contrib/mips-msa
While I looked into its configure file (starts at line 13610) for more detail about this.
https://github.com/glennrp/libpng/blob/a37d4836519517bdce6cb9d956092321eca3e73b/configure
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.
Those links don't work for me :/
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.
Fixed.
Thanks! |
Add support for mips64.
What does your PR fix?
fix build failure for mips64.
Which triplets are supported/not supported? Have you updated the CI baseline?
linux, No
Does your PR follow the maintainer guide?
Basically.
If you have added/updated a port: Have you run
./vcpkg x-add-version --all
and committed the result?Yes
If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/