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

Statically link compiler-rt #976

Closed
wants to merge 18 commits into from
Closed

Conversation

oliverhu
Copy link

@oliverhu oliverhu commented Aug 7, 2023

This PR addesses issues in #834 and tinygrad/tinygrad#1367, in which libllvmlite.so fails to resolve symbols in libclang_rt.builtin.a since compiler rt is not linked. This PR statically links the compiler_rt library.

This PR is based on top of #909 from @testhound

Validated both cmake & Makefile work on Ubuntu.

❯ readelf -s libllvmlite.so| grep f2h
   600: 0000000003dcefaf    32 FUNC    GLOBAL DEFAULT   12 __gnu_f2h_ieee
194927: 0000000003dcefaf    32 FUNC    GLOBAL DEFAULT   12 __gnu_f2h_ieee

I did not test in Windows

@gmarkall
Copy link
Member

gmarkall commented Aug 7, 2023

Hi @oliverhu - many thanks for the PR continuing this work! We'll discuss assigning it at the triage meeting tomorrow.

@gmarkall gmarkall requested review from sklam and gmarkall August 8, 2023 14:28
@gmarkall
Copy link
Member

gmarkall commented Aug 8, 2023

From the discussion in Gitter and in the triage meeting, the path forward to the review / testing / merge of this looks like:

  1. Create a new PR with only the llvmdev recipe changes (the changes in buildscripts/conda-recipes - the change to llvmlite's meta.yaml is unneeded though). (for @oliverhu)
  2. We (either @esc or myself, whoever gets to it first) will then build llvmdev packages from the PR in step 1 and put it on a separate channel. This could potentially be a personal channel to avoid needing it to be done by someone who has access to upload to the numba Anaconda organization. I do think think a maintainer should build this package (rather than @oliverhu), so that we have in some sense tested / reviewed it rather than relying on the package built by a contributor.
  3. This PR should remove the llvmdev package changes, and be modified to use the package built and published on the channels in step 2.
  4. We (llvmlite maintainers) then review / test this PR, and if all looks good, we merge the PR from step 1 for the llvmdev package changes.
  5. Following that merge, the llvmdev package gets rebuilt and put on the official channels with a new build number.
  6. This PR should then be modified to use the package from the official channel again (and not the one from step 2).
  7. This PR can then be run through CI again to check all is good, and then merged.

@oliverhu Does this plan make sense to you, and would you be happy to go ahead with splittling out the llvmdev package changes for us to move forward?

@oliverhu
Copy link
Author

oliverhu commented Aug 8, 2023

Sounds good.

@gmarkall
Copy link
Member

I'm uploading packages to the gmarkall account on anaconda.org: https://anaconda.org/gmarkall/llvmdev/files

To proceed with this PR:

  • Could you please remove all the conda recipe changes (addressed by Update llvmdev to build compiler-rt #979)
  • There don't appear to be any tests - can you think of a test (or multiple tests) we can add that will demonstrate that the compiler-rt builtins are present and working?
  • There's no documentation - can you add some notes to the documentation explaining anything relevant for users? E.g. what's available through the compiler-rt builtins, how to use them if necessary, and when they might be needed? The docs should also state that this is an experimental feature for the current release - since this is a bit new to llvmlite, I think we should have a release where we consider it experimental then remove the "experimental" designation if it seems to be OK in use after a release.
  • The copying of the libs in the CMakeLists.txt file seems a bit odd, and I can't understand why it's necessary - why not just figure out where the compiler-rt builtins live and link directly to them? Can this copying be eliminated?
  • The Visual Studio 17 additions to build.py also seem unnecessary, since we didn't use it to built llvmdev - can this be removed without side-effects?

The above are the items that I can see on first glance - hopefully we can have another iteration after looking at the above which will be a little simpler!

Many thanks again for your efforts here!

@oliverhu
Copy link
Author

Closing this one, we can follow up in #986

@oliverhu oliverhu closed this Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants