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

Replace vendored SIMD Everywhere by prefix/system install #10961

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Jul 9, 2024

Description

Important

Depends on:

Close #5067

Replace vendored SIMD Everywhere by prefix/system install.

SIMD Everywhere finder needs to be installed alongside libobs CMake
package since its headers depends on it.

macOS AVX intrinsics is included to make sure that AVX defines are
present.

Motivation and Context

Reduce in-tree vendored deps in OBS Studio repo.

How Has This Been Tested?

Build in CI on my fork.

Types of changes

  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@tytan652 tytan652 requested a review from PatTheMav July 9, 2024 16:31
@tytan652 tytan652 added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable labels Jul 9, 2024
@tytan652 tytan652 force-pushed the simd_everywhere_except_in_this_repo branch 2 times, most recently from efd9e74 to 3985104 Compare July 9, 2024 16:40
@tytan652 tytan652 force-pushed the simd_everywhere_except_in_this_repo branch from 3985104 to 2ac6089 Compare July 15, 2024 17:44
@@ -426,6 +426,14 @@ function(target_export target)
DESTINATION "${package_destination}"
COMPONENT Development
${exclude_variant})

if(target STREQUAL libobs)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we also need to export SIMDE here? Are SIMDE types used in function prototypes or inline functions?

Copy link
Collaborator Author

@tytan652 tytan652 Jul 22, 2024

Choose a reason for hiding this comment

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

The headers include a SIMDE header which make it a dependency of libobs header and so the CMake package needs the finder to be installed alongside the package.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the question was more about which ones? Because SIMDE is an implementation detail of libobs internals and should not be exposed publicly. It doesn't make sense to expose it from an API point of view and if we do that seems like a mistake to me.

Copy link
Collaborator Author

@tytan652 tytan652 Jul 22, 2024

Choose a reason for hiding this comment

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

util/sse-intrin.h (which hard depends on SIMDE) used in some installed headers:

grep -r "sse-intrin.h" /usr/include/obs/. 
/usr/include/obs/./graphics/quat.h:#include "../util/sse-intrin.h"
/usr/include/obs/./graphics/vec3.h:#include "../util/sse-intrin.h"
/usr/include/obs/./graphics/vec4.h:#include "../util/sse-intrin.h"

Copy link
Member

Choose a reason for hiding this comment

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

Right and of course we inlined all those functions so we created a tight coupling between API users and the internal data types..

That should have never happened, but I guess that's water under the bridge and can be cleaned up once the current API is not used externally anymore in the future.

@tytan652 tytan652 force-pushed the simd_everywhere_except_in_this_repo branch 3 times, most recently from 56c0582 to a86a165 Compare August 16, 2024 18:04
@tytan652 tytan652 marked this pull request as ready for review August 16, 2024 18:05
@tytan652 tytan652 force-pushed the simd_everywhere_except_in_this_repo branch from a86a165 to 2736151 Compare August 28, 2024 18:32
SIMD Everywhere finder needs to be installed alongside libobs CMake
package since its headers depends on it.

macOS AVX intrinsics is included to make sure that AVX defines are
present.
@tytan652 tytan652 force-pushed the simd_everywhere_except_in_this_repo branch from 2736151 to ebbb3a1 Compare September 2, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Enhancement Improvement to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants