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 LLVM 10 #14

Merged
merged 2 commits into from Apr 1, 2020
Merged

Support LLVM 10 #14

merged 2 commits into from Apr 1, 2020

Conversation

QuLogic
Copy link
Contributor

@QuLogic QuLogic commented Feb 11, 2020

There are very few changes here, but as it's based on #12 and #13, it looks a bit bigger.

LLVM 10 headers require C++14 or compilation fails, so this changes the config file to ask for it instead of C++11.

The removal of FlagBlockByrefStruct is from llvm/llvm-project@0779dff but it's unused in tinygo anyway.

The additional parameter in LLVMDIBuilderCreateTypedef is from llvm/llvm-project@f1e3988, but formatted correctly and with backwards-compatibility code for LLVM 9.

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 11, 2020

This builds against LLVM 10.0.0-rc1, so you may wish to not merge until a final release is out, in case there are any last minute changes.

@aykevl
Copy link
Member

aykevl commented Feb 12, 2020

What is the intention of this PR? Do you want to support both LLVM 9 and LLVM 10 at the same time in TinyGo or is this simply preparing for when we switch to LLVM 10 support?

@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 13, 2020

Preferably supporting both. Fedora gets a new major release of LLVM only on every release (F30 had 8, F31 had 9, F32 will have 10, etc.), so if TinyGo drops support for old LLVM, it cuts out a whole Fedora release, and I can't update it (except by patching, which is admittedly small so far.)

@aykevl
Copy link
Member

aykevl commented Feb 18, 2020

Okay, sounds reasonable. AFAIK there were no major changes in LLVM 10 that we want to use, so I think it would be possible to support both at the same time.

I would suggest that once we support LLVM 10 in TinyGo, all builders will switch over to LLVM 10 except for the one that also tests Go 1.11 (test-llvm9-go113 in CircleCI). It would make that test effectively the "lower bound" that is supported.

This builds against LLVM 10.0.0-rc1, so you may wish to not merge until a final release is out, in case there are any last minute changes.

I would like to wait until the final release is out, as it makes testing a lot easier.

Comment on lines +51 to +55
@if [ $(VERSION_MAJOR) -gt 9 ]; then \
echo "// #cgo CXXFLAGS: -std=c++14" >> llvm_config_$(GOOS).go; \
else \
echo "// #cgo CXXFLAGS: -std=c++11" >> llvm_config_$(GOOS).go; \
fi
Copy link
Member

Choose a reason for hiding this comment

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

I tested with -std=c++14 for LLVM 9 and it seems to work just fine, so this condition should not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I doubt that it would break LLVM 9, but I left it there in case old systems didn't support it (but could still build LLVM with c++11).

llvm_config_linux.go Outdated Show resolved Hide resolved
backports.cpp Outdated Show resolved Hide resolved
@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 24, 2020

This builds against LLVM 10.0.0-rc1, so you may wish to not merge until a final release is out, in case there are any last minute changes.

I would like to wait until the final release is out, as it makes testing a lot easier.

Feel free to merge #12 and #13 in the meantime.

@aykevl
Copy link
Member

aykevl commented Feb 26, 2020

Feel free to merge #12 and #13 in the meantime.

Done. See tinygo-org/tinygo#924 to update the go-llvm dependency, is that useful for packaging?

Also, note that supporting both LLVM 9 and LLVM 10 will need some more changes to TinyGo. It might even need a build tag (llvm9 or llvm10).

The latest LLVM seems to require it, as several symbols are undefined
otherwise.
@QuLogic
Copy link
Contributor Author

QuLogic commented Feb 26, 2020

Done. See tinygo-org/tinygo#924 to update the go-llvm dependency, is that useful for packaging?

Fedora doesn't yet use Go modules, so it's not as important just yet, but it is nice to reduce differences between what we build and what upstream builds.

@aykevl
Copy link
Member

aykevl commented Mar 3, 2020

I have an unfinished llvm10 branch that doesn't yet compile in CI, I intend to fix that once LLVM 10 is released.

Does the Fedora version of TinyGo link statically or dynamically to LLVM? That is important for how version-independence is best achieved (it's much easier when linking statically).

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 4, 2020

Builds are always dynamic, but as mentioned earlier, major bumps don't really happen except at major releases (and thus everything would be rebuilt anyway.)

@QuLogic
Copy link
Contributor Author

QuLogic commented Mar 4, 2020

I do like the change on that branch though, as it's one less thing for me to patch.

@aykevl
Copy link
Member

aykevl commented Mar 4, 2020

I'm asking because linking dynamically will need more changes. In particular, the LLVM version is also included in cgo/libclang_config.go. So what I think would work best is that everything gets updated to LLVM 10 but we add some new files (tinygo/cgo/libclang_config_llvm9.go, go-llvm/llvm_config_linux_llvm9.go) that get built with a new llvm9 build tag. That should mean less work for distros that want to remain compatible with an older LLVM release while everyone else gets the improvements made in the new LLVM release. Would that help you?
With support for an older LLVM release it should also be possible to test such old releases in CI. For example, I intend to run the Go 1.11 CI with LLVM 9 instead of LLVM 10 after the LLVM 10 update, so that we don't accidentally break the compiler.

That said, I can't guarantee everything will keep working in newer LLVM releases. For example, I've been investigating how feasible it is to do automatic stack size calculations for some easy cases and ideally I'd like to use a new DWARF feature supported only in LLVM 10. This feature should fall back gracefully however.

Sidenote: we're a bit past the scheduled LLVM 10 release so it can happen any day now.
EDIT: as it happens, in the same minute as I posted this comment RC3 of LLVM got released.

@aykevl aykevl merged commit cf0cde0 into tinygo-org:master Apr 1, 2020
@aykevl
Copy link
Member

aykevl commented Apr 1, 2020

I have merged this branch as I believe it's fully backwards compatible.

@QuLogic QuLogic deleted the llvm10 branch April 1, 2020 20:19
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

2 participants