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 portable build without intrinsics #261

Merged
merged 2 commits into from Oct 12, 2022
Merged

Support portable build without intrinsics #261

merged 2 commits into from Oct 12, 2022

Conversation

wargio
Copy link
Contributor

@wargio wargio commented Sep 17, 2022

It is impossible to build currently if you want to have a pure-portable build of the C code.
When we build rizin, we need to support TinyCC which does not have immintrin.h nor intrin.h.
This patch removes some dup declarations and defines the default values for the missing intrinsics.

@oconnor663
Copy link
Member

Sounds reasonable to me. @sneves let me know what you think.

@wargio how about including something like this to exercise TCC in CI:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index d7d754f..fe6d2fc 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -233,3 +233,17 @@ jobs:
     - name: build b3sum
       run: cargo build --target aarch64-apple-darwin
       working-directory: ./b3sum
+
+  build_tinycc:
+    name: build with the Tiny C Compiler
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v1
+    - name: install TCC
+      run: apt-get install -y tcc
+    - name: compile
+      run: >
+        tcc -shared -O3 -o libblake3.so
+          -DBLAKE3_NO_SSE2 -DBLAKE3_NO_SSE41 -DBLAKE3_NO_AVX2 -DBLAKE3_NO_AVX512
+          blake3.c blake3_dispatch.c blake3_portable.c
+      working-directory: ./c

@sneves
Copy link
Collaborator

sneves commented Oct 1, 2022

I meant to reply to this at the time and then just forgot. I think the minimal viable patch to solve this is

diff --git a/c/blake3_dispatch.c b/c/blake3_dispatch.c
index b498058..f8f7400 100644
--- a/c/blake3_dispatch.c
+++ b/c/blake3_dispatch.c
@@ -10,7 +10,7 @@
 #elif defined(__GNUC__)
 #include <immintrin.h>
 #else
-#error "Unimplemented!"
+#undef IS_X86 /* Unimplemented */
 #endif
 #endif
 
diff --git a/c/blake3_impl.h b/c/blake3_impl.h
index ba2e91c..ad3940e 100644
--- a/c/blake3_impl.h
+++ b/c/blake3_impl.h
@@ -46,7 +46,6 @@ enum blake3_flags {
 #if defined(_MSC_VER)
 #include <intrin.h>
 #endif
-#include <immintrin.h>
 #endif

@wargio
Copy link
Contributor Author

wargio commented Oct 3, 2022

@oconnor663 and @sneves i have applied the changes you have suggested.

@oconnor663
Copy link
Member

@wargio apologies, the apt-get install command needs sudo. If you know of a way for me to configure things so CI just runs without waiting for me to hit the button, please let me know.

@wargio
Copy link
Contributor Author

wargio commented Oct 11, 2022

@wargio apologies, the apt-get install command needs sudo. If you know of a way for me to configure things so CI just runs without waiting for me to hit the button, please let me know.

Unfortunately there is no way since this is my first PR to this repo, but since i'm right now online, you can press it again :) and i can fix issues

@oconnor663
Copy link
Member

CI is green! @sneves does this still look good to you?

@sneves
Copy link
Collaborator

sneves commented Oct 12, 2022

Sure

@oconnor663 oconnor663 merged commit f84636e into BLAKE3-team:master Oct 12, 2022
@wargio wargio deleted the pure-portable branch October 12, 2022 08:20
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.

None yet

3 participants