-
Notifications
You must be signed in to change notification settings - Fork 19
Feat: better clang support #309
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
base: fixing_indirections
Are you sure you want to change the base?
Feat: better clang support #309
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.
Pull Request Overview
This PR improves Clang compiler compatibility for the Gibbon runtime system by adding compiler-agnostic macro wrappers and toolchain detection. The changes address issue #308 by ensuring the codebase can compile cleanly with both GCC and Clang.
Key changes:
- Introduces cross-compiler pragma macros (
GIB_PRAGMA_MESSAGE,GIB_PRAGMA_UNROLL) to handle compiler-specific syntax differences - Adds automatic detection of Clang compiler and selection of appropriate archiver tool (llvm-ar vs gcc-ar)
- Replaces GCC-specific inline assembly syntax with portable
__asm__ __volatile__form
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gibbon-rts/rts-c/gibbon_rts.h | Adds pragma wrapper macros and updates inline assembly syntax for cross-compiler compatibility |
| gibbon-rts/rts-c/gibbon_rts.c | Replaces direct pragma usage with new wrapper macros throughout the runtime |
| gibbon-rts/Makefile | Adds -Werror flag and uthash include path configuration |
| gibbon-compiler/src/Gibbon/HaskellFrontend.hs | Removes redundant CPP conditionals around import statement |
| gibbon-compiler/src/Gibbon/Compiler.hs | Implements Clang detection and automatic archiver selection for build process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ulysses4ever
left a comment
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.
It's be wise to set up a test that runs in CI.
What kind of tests should I add? |
I believe all the tests run with gcc atm. |
|
You can set up a smoke test separately from everything else (e. g. a separate ci workflow but maybe not, needs thinking). It could compile some simple program with clang. What Vidush suggests is bonus points (but sounds great!). |
466991a to
90b43ad
Compare
|
I did what @vidsinghal suggested and now our tests run twice, once for gcc and once for clang. I also rebased my branch. |
90b43ad to
0d7e935
Compare
|
@Noir01 can you rebase this with fixing_indirections? Tests should pass now |
Not exactly sure why it was conditionalized but I kept running into an error and 'unconditionalizing' worked.
GIB_PRAGMA will emit build-time messages in the same manner as before but safely across Clang and GCC. I had Codex suggest this code, but I did my due diligence and tested and looked into official docs to ensure the solution works and actually makes sense.
Enable -Werror since we've fixed the errors in prev commits Also explicitly bundle uthash
Add an opt-in --cc={compiler} which is threaded throughout
all compiler invocations.
0d7e935 to
e7eb02a
Compare
|
builds failing |
The only usage of foldl' seems to be with L.foldl' on line 1412
|
@noir tests aren't setup properly? |
|
I just looked at the logs, and what I guess is happening is that I didn't add llvm-ar in the workflow image, so every time it runs the tests and compares the output, the warning we added for different major versions of clang and llvm-ar triggers which is tripping up the simple diff tests. |
we need to update the workflow image to include llvm-ar |
This should prevent tests failing if llvm-ar and a fallback like /usr/bin/ar is used instead.
|
Regarding |
Oh, I wasn't aware of that. How do I use the existing infrastructure? |
|
I'm not sure there is. I might have mixed that up with err: gibbon/gibbon-compiler/src/Gibbon/Common.hs Line 366 in 40d511e
It's a good idea to grep for stderr to check. |
|
I did Also, some of the tests are failing with segfaults for the clang workflows. I will try to debug them tonight/tomorrow morning. |
oh wow, this is interesting, i would be interested to know why, if you can't figure out lmk, i might try to investigate. Otherwise we will have to mark these failing for Clang. |
fixes #308