-
Notifications
You must be signed in to change notification settings - Fork 206
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
Initial code to support llvm-20 #684
base: master
Are you sure you want to change the base?
Conversation
I also tested llvm-19. That one compiles just fine with these changes, but running it ends up with the infamous message:
I built it against a custom compiled llvm-19 following similar steps as in #678, so I am not sure what is causing that. |
Thanks for working on this! I don't have much bandwidth to help at the moment, but I'll try to answer questions where I can.
If you scroll up you'll see the comment says it comes from I submitted an issue asking for them to make a public version, which they seem to have ignored: llvm/llvm-project#67076 It's too bad that there has been churn here. But unless you're aware of a different solution, we're stuck with copy-and-pasting the function and ifdef-ing it as necessary to make it apply to all versions. |
We could potentially take a version of this PR with only LLVM 19 changes. That would be personally useful to me (and maybe other members of the community).
Unfortunately I'm still not sure what to say about this. I imagine it's not really version-related. I could build LLVM 19 on my end, but I don't think that would fix it? |
Could you try? Just so that I can rule out if it is something related to my environment. In which case I can probably narrow down what is going on a bit. |
Well, I am not surprised :D. Thank you for these details. Sadly I know very little of llvm internals, so I can try to work something out for few hours, but I will bail out and ignore llvm20 if I see it is creeping too much :). To be honest, I dream one day of something like terra but based on a more lightweight backend, like qbe. At least, if I find bugs or regressions, the code-base is tame enough I can solve them and patch upstream. I cannot say the same of llvm :(. |
All reports I found suggest this problem is due to llvm being double linked somewhere. |
LLVM 19 binaries are here: https://github.com/terralang/llvm-build/releases/tag/llvm-19.1.7 I pushed changes into this branch to enable LLVM 19 CI on Linux, Mac and FreeBSD. Looks like we're hitting the same failure mode: https://github.com/terralang/terra/actions/runs/13205235502/job/36866760732?pr=684 So I guess the next step is for me to debug this locally. |
Honestly, I agree. I don't know this backend specifically, but if we had a backend that was stable, well-supported, and didn't make Terra harder to maintain, I'd be happy to merge it. There are things I can only do with LLVM, but keeping up with LLVM updates is such a pain.
Now that it's replicated I'll start debugging it. In the past it was related to mixing static and dynamic objects. But you can see in the binaries yourself that there are no |
You can see my initial attempt to fix this in 6cb9edb. My hunch there was that LLVM was accidentally including either (a) some object, or (b) some specific item in multiple I currently see two problems. First, Linux is still broken and requires an additional patch to work. The patch is here: diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 1a8e114..6a4f4de 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -347,16 +347,34 @@ elseif(UNIX)
"-Wl,-rpath,$ORIGIN/../lib"
)
endif()
+ if(TERRA_WHOLE_ARCHIVE_LLVM)
+ target_link_libraries(TerraExecutable
+ PRIVATE
+ -Wl,-export-dynamic
+ -Wl,--whole-archive
+ )
+ endif()
+
target_link_libraries(TerraExecutable
PRIVATE
- -Wl,-export-dynamic
- -Wl,--whole-archive
TerraLibrary
-Wl,--no-whole-archive
${LUAJIT_LIBRARIES}
${ALL_LLVM_LIBRARIES}
${LLVM_SYSTEM_LIBRARIES}
)
+ if(TERRA_WHOLE_ARCHIVE_LLVM)
+ target_link_libraries(TerraExecutable
+ PRIVATE
+ -Wl,--no-whole-archive
+ )
+ endif()
+ target_link_libraries(TerraExecutable
+ PRIVATE
+ ${LUAJIT_LIBRARIES}
+ ${ALL_LLVM_LIBRARIES}
+ ${LLVM_SYSTEM_LIBRARIES}
+ )
else()
target_link_libraries(TerraExecutable
PRIVATE But I have not pushed this patch because it actually does not fix the issue. On macOS (and Linus with this patch), I get:
This makes me think that we really do need the whole-archive link. However, maybe we need to exclude whole-archive from specific LLVM Unfortunate I've exceeded my available debugging time so that's all I can do for the moment. If you find more time to work on this, feel free to push patches, and I'll approve those to run so you can see CI results. |
I would not consider it a fair replacement/integration to be honest. It is way less feature rich (by design) and would break any downstream code that is rightfully based on llvm intrinsics as it does not strive for any llvm compatibility. Thanks for your tests, I will give it a second try next week. Hopefully I can sort something out. |
I started working on support for llvm20, since the interface changed in some places.
At the moment there are few things still missing which I have not been able to figure out:
as the interface of
createDiagnostics
had some significant changes and is now based on a virtual file system (?)as this structure totally disappeared from upstream for what I can tell. No idea on which replacement was designed.
Finally, a minor issue with
CloneBasicBlock
which saw an argument removed and I am not sure if I patched it up correctly.