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

[openmvs] add arm support #33101

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

autoantwort
Copy link
Contributor

Currently it fails because SSE is also enables on the arm platform

Comment on lines 9 to 11
-SET(OpenMVS_USE_SSE ON CACHE BOOL "Enable SSE optimizations")
+cmake_policy(SET CMP0127 NEW)
+cmake_dependent_option(OpenMVS_USE_SSE "Enable SSE optimizations" ON "NOT CMAKE_SYSTEM_PROCESSOR MATCHES \"^(arm|ARM|aarch64|AARCH64).\"" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This patches out what is aleady an option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From upstream: cdcseacave/openMVS#1042

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

What should we do with this PR next? Are we waiting for the upstream to apply the dg0yt's suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Cheney-W You are the reviewer. It is up to you give guidance whether to avoid the patch because it can be avoided, or to simply accept the patch for being accepted upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the alternative is to check the VCPKG_TARGET_ARCHITECTURE and set OpenMVS_USE_SSE accordingly. The result is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

@autoantwort Could you please apply this modification to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Cheney-W Done

@Cheney-W Cheney-W self-assigned this Aug 11, 2023
@Cheney-W Cheney-W added the category:port-bug The issue is with a library, which is something the port should already support label Aug 11, 2023
Cheney-W
Cheney-W previously approved these changes Aug 15, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 24, 2023
@dan-shaw dan-shaw merged commit f8c8cf6 into microsoft:master Aug 24, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants