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

support multiple architectures (e.g. SSE, AVX) in the same executable… #118

Conversation

JeffMcClintock
Copy link
Contributor

… via a macro

@JeffMcClintock JeffMcClintock force-pushed the simultaneous_multiple_architectures branch 5 times, most recently from 1562007 to 2b52367 Compare November 21, 2023 23:18
@JeffMcClintock JeffMcClintock force-pushed the simultaneous_multiple_architectures branch from 2b52367 to e5e2cce Compare November 21, 2023 23:22
Comment on lines 50 to 53
target_compile_definitions(RTNeural
PRIVATE
RTNEURAL_NAMESPACE=RTNeural
)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
target_compile_definitions(RTNeural
PRIVATE
RTNEURAL_NAMESPACE=RTNeural
)
set(RTNEURAL_NAMESPACE "RTNeural" CACHE STRING "Namespace to use for RTNeural code")
target_compile_definitions(RTNeural
PUBLIC
RTNEURAL_NAMESPACE=${RTNEURAL_NAMESPACE}
)

I was thinking it might make sense to do this through a CMake variable, just to be a little more transparent?
I think I'd also prefer the namespace definition to be public so that user-space code can also use RTNEURAL_NAMESPACE if needed, but maybe that could cause other issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. Done.

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (37fde44) 95.64% compared to head (fab98d1) 95.64%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #118   +/-   ##
=======================================
  Coverage   95.64%   95.64%           
=======================================
  Files          58       58           
  Lines        3860     3860           
=======================================
  Hits         3692     3692           
  Misses        168      168           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,4 +1,4 @@
CXXFLAGS = -O3 -I../../ -std=c++17 -DRTNEURAL_DEFAULT_ALIGNMENT=16
CXXFLAGS = -O3 -I../../ -std=c++17 -DRTNEURAL_DEFAULT_ALIGNMENT=16 -DRTNEURAL_NAMESPACE=RTNeural
Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I hadn't thought about this... maybe instead it would be better to have something at the top of "RTNeural.h" like:

#ifndef RTNEURAL_NAMESPACE
#define RTNEURAL_NAMESPACE RTNeural
#endif

That way folks currently using RTNeural without CMake won't have a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

smart. I've pushed a new commit.

@jatinchowdhury18
Copy link
Owner

Awesome, thanks again! Merging now...

@jatinchowdhury18 jatinchowdhury18 merged commit 1e7b570 into jatinchowdhury18:main Nov 23, 2023
21 checks passed
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