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

Update Brillvm to Llvm 18 #319

Merged
merged 17 commits into from
May 27, 2024
Merged

Update Brillvm to Llvm 18 #319

merged 17 commits into from
May 27, 2024

Conversation

Pat-Lafon
Copy link
Contributor

A pr to move brillvm to LLVM 18.

The LLVM version is sorta a moving target. Currently brillvm is based on LLVM 16 but rustc has moved passed this (currently llvm 17 but it's updating to llvm 18 in the next release https://releases.rs/docs/1.78.0/). I'm not sure how much breaks between LLVM versions but I don't want to fall too far behind.

Brew recently add LLVM 18 which sparked this pr. Inkwell has support but is waiting for some of their ci infrastructure to catch up before cutting a new release I think.

Things seem to work locally even with rustc still using LLVM 17?

@Pat-Lafon
Copy link
Contributor Author

Apparently, someone has stopped making LLVM linux x86 binaries since llvm17. 🤷 The macos-latest runners are m1 and not x86 afaik.

@Pat-Lafon
Copy link
Contributor Author

Pat-Lafon commented Apr 26, 2024

KyleMayes/install-llvm-action#70 (comment) Seems like this pr is a bit stalled for now, sorry for any pings from workflow testing.

@sampsyo
Copy link
Owner

sampsyo commented Apr 29, 2024

Always a tricky thing! Thanks for giving it a shot… we'll see if the CI stuff works itself out.

@@ -72,6 +72,11 @@ pub unsafe extern "C" fn _bril_parse_float(arg: *const c_char) -> f64 {
r_str.parse::<f64>().unwrap()
}

#[no_mangle]
pub extern "C" fn _start() -> ! {

Choose a reason for hiding this comment

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

On Linux if I compile a test bril program with this version of the runtime with clang-18 I get a /usr/bin/ld: duplicate definition of _start error.

Commenting this out and rebuilding the runtime has made it go away. I'm unsure if I'm missing some sort of linker option when I compile with clang, which would just be a documentation error.

Any way I can help with this? I can document/provide patch etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I took a couple of roundabouts while addressing your issue and I thought I still needed this for compilation. Seems we don't. Thanks for bringing this up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated but if you have any wisdom for getting an llvm installation in a github worker(especially for llvm 18) for testing that would be very useful.

Choose a reason for hiding this comment

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

Hmmmmm, I have lots of github actions experience, a decent amount of LLVM experience, but no experience together.

The two projects I can point you to of examples of CI for LLVM for projects I have touched:

  1. Alive2 works off of trunk, so it should be good to use with LLVM 18.
  2. TinyGo Lots more complicated and they work off of a fork of LLVM, but lots of caching as well so they aren't having to set up LLVM all the time

If you want to mirror anyone's I'd recommend mirroring Alive's

@Pat-Lafon
Copy link
Contributor Author

Yay! I've given up on trying to download LLVM binaries from LLVM itself. Over time they have been stripping down the binaries such that some releases didn't have Poly or llvm-config. There also seems to be a looming restructuring with LLVM 19 for security/just general inconsistency in which versions get built for which platforms.

The solution is to pull their blessed llvm.sh automation script which handles the version/package resolution with apt. A copy-and-paste from https://apt.llvm.org/

@Pat-Lafon
Copy link
Contributor Author

I can clean up the history as needed. Also note that I've snuck in the linking fixes and handling of multiple phi nodes for brillvm as well. I'm reasonably satisfied with the result unless @ryan-berger has noticed any issues.

@sampsyo
Copy link
Owner

sampsyo commented May 22, 2024

Awesome!! All looks good from here. Super cool that this works without too much fuss in CI.

If @ryan-berger can assure us that we wouldn't break anything on their side, I say we merge it!

@ryan-berger
Copy link

@Pat-Lafon we forked off this branch, so let me have @oflatt update it and then we can see if everything works.

Everything before the multiple phi commit works for sure though because we were using it

@ryan-berger
Copy link

@Pat-Lafon had a conversation with Oliver. I wouldn't wait on us as we now (as of a few weeks ago) explicitly generate code with non ssa'd bril so that we don't encounter phis. Our IR no longer contains phis, so we can't generate them. We let mem2reg take it from there.

@sampsyo
Copy link
Owner

sampsyo commented May 27, 2024

OK, sounds good! Let's merge this up now.

@sampsyo sampsyo merged commit a9925d5 into sampsyo:main May 27, 2024
17 checks passed
@Pat-Lafon Pat-Lafon deleted the llvm-18 branch May 27, 2024 20:03
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