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

Bump LLVM to v18 #54848

Merged
merged 21 commits into from
Aug 4, 2024
Merged

Bump LLVM to v18 #54848

merged 21 commits into from
Aug 4, 2024

Conversation

Zentrik
Copy link
Member

@Zentrik Zentrik commented Jun 18, 2024

No description provided.

@Zentrik Zentrik closed this Jun 18, 2024
@Zentrik Zentrik added external dependencies Involves LLVM, OpenBLAS, or other linked libraries compiler:llvm For issues that relate to LLVM labels Jun 18, 2024
@Zentrik Zentrik changed the title Bump LLVM to v18 [TEST] Bump LLVM to v18 Jun 20, 2024
@Zentrik Zentrik reopened this Jun 20, 2024
src/disasm.cpp Outdated Show resolved Hide resolved
@giordano giordano added building Build system, or building Julia or its dependencies JLLs labels Jun 20, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Jun 20, 2024

Bit confused why windows won't start building

@giordano
Copy link
Contributor

I think there's something broken with the CI setup. CC: @staticfloat

@gbaraldi
Copy link
Member

This fixes the llvmpasses tests

diff --git a/test/llvmpasses/pipeline-prints.ll b/test/llvmpasses/pipeline-prints.ll
index babd26c797..ecb7095302 100644
--- a/test/llvmpasses/pipeline-prints.ll
+++ b/test/llvmpasses/pipeline-prints.ll
@@ -298,12 +298,12 @@ attributes #2 = { inaccessiblemem_or_argmemonly }

 ; COM: Loop simplification makes the exit condition obvious
 ; AFTERLOOPSIMPLIFICATION: L35.lr.ph:
-; AFTERLOOPSIMPLIFICATION-NEXT: add nuw nsw
+; AFTERLOOPSIMPLIFICATION: add nuw nsw

 ; COM: Scalar optimization removes the previous add from the preheader
-; AFTERSCALAROPTIMIZATION: L35.preheader:
+; AFTERSCALAROPTIMIZATION: L35.lr.ph:
 ; AFTERSCALAROPTIMIZATION-NOT: add nuw nsw
-; AFTERSCALAROPTIMIZATION-NEXT: br label %L35
+; AFTERSCALAROPTIMIZATION: br label %L35

 ; COM: Vectorization does stuff
 ; AFTERVECTORIZATION: vector.body

@vtjnash
Copy link
Member

vtjnash commented Jun 24, 2024

llvm/llvm-project#78251

@gbaraldi
Copy link
Member

gbaraldi commented Jul 2, 2024

The windows build is segfaulting in

#0  0x00007ff8e4f70de5 in ntdll!RtlVirtualUnwind () from /c/Windows/SYSTEM32/ntdll.dll
#1  0x00007ff8e4f70ccb in ntdll!RtlVirtualUnwind () from /c/Windows/SYSTEM32/ntdll.dll
#2  0x00007ff8e3ee3bd8 in RtlVirtualUnwind () from /c/Windows/System32/KERNEL32.DLL
#3  0x00007ff8bb29cfed in jl_unw_step (cursor=0xba5690, from_signal_handler=0, ip=<synthetic pointer>,
    sp=<synthetic pointer>) at C:/msys64/home/vboxuser/julia/src/stackwalk.c:523
#4  jl_unw_stepn (cursor=cursor@entry=0xba5690, bt_data=bt_data@entry=0x42b0080, bt_size=bt_size@entry=0xba51b8,
    sp=sp@entry=0x0, maxsize=maxsize@entry=80000, skip=0, skip@entry=3, ppgcstack=ppgcstack@entry=0xba51b0,
    from_signal_handler=from_signal_handler@entry=0) at C:/msys64/home/vboxuser/julia/src/stackwalk.c:99
#5  0x00007ff8bb29d752 in rec_backtrace (bt_data=0x42b0080, maxsize=maxsize@entry=80000, skip=skip@entry=2)
    at C:/msys64/home/vboxuser/julia/src/stackwalk.c:222
#6  0x00007ff8bb264313 in record_backtrace (ptls=0x3d71d10, skip=skip@entry=1)
    at C:/msys64/home/vboxuser/julia/src/task.c:412
#7  0x00007ff8bb264a85 in ijl_throw (e=0x13b7c0c0) at C:/msys64/home/vboxuser/julia/src/task.c:771
#8  0x00007ff8bb2cfdb9 in ijl_errorf (
    fmt=fmt@entry=0x7ff8bb482178 <flisp_system_image+362104> "Type %s does not have a definite size.")
    at C:/msys64/home/vboxuser/julia/src/rtutils.c:77
#9  0x00007ff8bb24d3a8 in jl_f_sizeof (F=<optimized out>, args=<optimized out>, nargs=<optimized out>)
    at C:/msys64/home/vboxuser/julia/src/builtins.c:531

@giordano
Copy link
Contributor

giordano commented Jul 2, 2024

Was libllvm_jll updated to 18.1.7+2? That includes llvm/llvm-project#88172 which should fix WIndows issues according to JuliaLang/llvm-project#30 (comment).

@gbaraldi
Copy link
Member

gbaraldi commented Jul 2, 2024

This doesn't have the new LLVM build yet. I was fixing the embedding thing mostly

@Zentrik
Copy link
Member Author

Zentrik commented Jul 2, 2024

Oops, 64 bit windows builds on the branch with head Zentrik/llvm-project@b557165 not with JuliaLang/llvm-project#30.

EDIT: These are the changes, JuliaLang/llvm-project@julia-release/18.x...Zentrik:llvm-project:windows-fix3. I just reverted a bunch of commits.

@gbaraldi
Copy link
Member

gbaraldi commented Jul 2, 2024

Do we know which one broke it?

@Zentrik
Copy link
Member Author

Zentrik commented Jul 2, 2024

I have no clue.

@Zentrik
Copy link
Member Author

Zentrik commented Jul 7, 2024

Looks like Zentrik/llvm-project@5d8cd98 is sufficient to get llvm 18 building on 64 bit windows (applied to https://github.com/JuliaLang/llvm-project/releases/tag/julia-18.1.7-1). The pr introducing the change, llvm/llvm-project#73037 suggests that the pr could cause trouble if we are assuming/hardcoding '.text'.

@gbaraldi
Copy link
Member

gbaraldi commented Jul 7, 2024

What is segfaulting here? Because I don't see how that breaks stuff. We just register functions with the JIT

@Zentrik
Copy link
Member Author

Zentrik commented Jul 7, 2024

This assertion triggers on a .ltext section

assert(!code_allocated);

@gbaraldi
Copy link
Member

gbaraldi commented Jul 7, 2024

But if we remove the assertion (it seems slightly weird anyway)

@Zentrik
Copy link
Member Author

Zentrik commented Jul 7, 2024

I didn't look into the segfaults at all so no idea. Pretty sure removing the assertion will just lead to another one being triggered, I recall at some point having https://github.com/llvm/llvm-project/blob/73447a3302541c343525570222b318e7f94f9402/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp#L299-L301 assert though I don't remember how I got to that point.

@Zentrik
Copy link
Member Author

Zentrik commented Jul 21, 2024

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@Zentrik
Copy link
Member Author

Zentrik commented Jul 21, 2024

Benchmarks lgtm assuming the regressions in scalar and collections on the order of ns are noise. An average .5% perf improvement excluding those.

giordano pushed a commit that referenced this pull request Jul 21, 2024
Split out from #54848 where this is necessary as `MAX_ALIGN` is now 16
on x86. Similar change to #34554.
@giordano
Copy link
Contributor

giordano commented Jul 22, 2024

assuming the regressions in scalar and collections on the order of ns are noise.

I ran locally some benchmarks on sincos and sinh (among the scalar functions with worse regressions) and I couldn't see any sensible difference (i.e. @benchmark showing subnanosecond differences for minimum times, and overall distributions with @benchmark were very similar as well) between this PR

julia> versioninfo()
Julia Version 1.12.0-DEV.899
Commit b9281e34aa8 (2024-07-21 16:17 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
  WORD_SIZE: 64
  LLVM: libLLVM-18.1.7 (ORCJIT, haswell)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

and latest nightly

julia> versioninfo()
Julia Version 1.12.0-DEV.882
Commit a1e0f5d921c (2024-07-22 01:06 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
  WORD_SIZE: 64
  LLVM: libLLVM-17.0.6 (ORCJIT, haswell)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)

@oscardssmith
Copy link
Member

is the generated code similar?

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@Zentrik
Copy link
Member Author

Zentrik commented Jul 23, 2024

I suspect the two packages failing due to invalid IR generated is due to llvm/llvm-project#63984.

@gbaraldi
Copy link
Member

gbaraldi commented Jul 23, 2024

The skipmissing regression is real if anyone wants a deeper look. I really wish BaseBenchmarks had an option to ask for the LLVM IR of the function being called. 😉

@Zentrik
Copy link
Member Author

Zentrik commented Jul 27, 2024

The skipmissing regression is real if anyone wants a deeper look. I really wish BaseBenchmarks had an option to ask for the LLVM IR of the function being called. 😉

The unoptimized LLVM IR looks the same to me, here's the optimized IR with LLVM 18 https://gist.github.com/Zentrik/b49a9d5b67dcfaae7b420ef8ed28df03 and on 0fd1f04 https://gist.github.com/Zentrik/e42698cb8db60932790f4f3657d35efb. The main difference seems to be a error branch isn't optimised out.
Here's the unoptimised IR https://godbolt.org/z/8ojePE9o9.

@gbaraldi
Copy link
Member

I think this is ready to merge. @vtjnash and @vchuravy any objections?

@gbaraldi gbaraldi added the merge me PR is reviewed. Merge when all tests are passing label Aug 2, 2024
@giordano giordano merged commit 2a56b78 into JuliaLang:master Aug 4, 2024
8 checks passed
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Aug 4, 2024
@vchuravy
Copy link
Member

vchuravy commented Aug 4, 2024

Congratulations! Awesome work everyone :)

lazarusA pushed a commit to lazarusA/julia that referenced this pull request Aug 17, 2024
Co-authored-by: Gabriel Baraldi <baraldigabriel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
building Build system, or building Julia or its dependencies compiler:llvm For issues that relate to LLVM external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants