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 v17 #53070

Merged
merged 86 commits into from
May 1, 2024
Merged

Bump LLVM to v17 #53070

merged 86 commits into from
May 1, 2024

Conversation

mofeing
Copy link
Contributor

@mofeing mofeing commented Jan 26, 2024

No description provided.

@giordano giordano added building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs labels Jan 26, 2024
@inkydragon
Copy link
Member

We've just upgrade to LLVM 16, 1 or 2 weeks ago (#51720).
That was a long trip, so good luck.

Welcome to the Julia community.

@mofeing
Copy link
Contributor Author

mofeing commented Jan 28, 2024

We've just upgrade to LLVM 16, 1 or 2 weeks ago (#51720). That was a long trip, so good luck.

Yes, I know. I was invited by @gbaraldi and @giordano to try this since getting LLVM_full_jll 17 to work was super fast.

Welcome to the Julia community.

Thanks! :)

@mofeing
Copy link
Contributor Author

mofeing commented Jan 28, 2024

@gbaraldi would you mind checking 66c13ac?

@mofeing
Copy link
Contributor Author

mofeing commented Jan 28, 2024

I just found out https://github.com/llvm/llvm-project/blob/23b233c8adad5b81e185e50d04356fab64c2f870/llvm/docs/OpaquePointers.rst#migration-instructions

The following commits might not be completely correct:

If we revise these commits and fix the remaining deprecation warnings (which I believe are all related to the Opaque pointers transition), then builds for all platform should succeed.

@gbaraldi
Copy link
Member

66c13ac seems fine, it seems they've changed the order these things need to happen, you can maybe even move the FAM down since it's no longer an argument so it's together with the other analysis managers

src/jitlayers.cpp Outdated Show resolved Hide resolved
@giordano
Copy link
Contributor

diff --git a/src/intrinsics.cpp b/src/intrinsics.cpp
index c784727c4f5..ff3c55bc072 100644
--- a/src/intrinsics.cpp
+++ b/src/intrinsics.cpp
@@ -4,6 +4,10 @@ namespace JL_I {
 #include "intrinsics.h"
 }
 
+#include <array>
+#include <bitset>
+#include <string>
+
 #include "ccall.cpp"
 
 //Mark our stats as being from intrinsics irgen

fixes compilation of src/codegen.cpp for me on gnu linux.

@giordano giordano added the compiler:llvm For issues that relate to LLVM label Jan 29, 2024
@vchuravy
Copy link
Member

@mofeing thank you for your work! One note from my side is that we aim to support the lastest LLVM version used by the latest Julia release for 1.10 that would be LLVM15. The goal is to be able to build for LLVM 15 without too many issues to be able to check where regressions came from.

This means that you may need to guard your changes under a JL_LLVM_VERSION >= 170000

Comment on lines +6 to +7
# Try to find a symbol from the C API of libLLVM as a simple sanity check.
@test dlsym(libLLVM_jll.libLLVM_handle, :LLVMContextCreate; throw_error=false) !== nothing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vchuravy Not sure how to make this code compatible with LLVM 15 back again. Is there some way to check the version of libLLVM_jll?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't LLVMContextCreate available in LLVM 15? When I blamed that function I found it was introduced 15 years ago.

@mofeing
Copy link
Contributor Author

mofeing commented Jan 29, 2024

@mofeing thank you for your work! One note from my side is that we aim to support the lastest LLVM version used by the latest Julia release for 1.10 that would be LLVM15. The goal is to be able to build for LLVM 15 without too many issues to be able to check where regressions came from.

This means that you may need to guard your changes under a JL_LLVM_VERSION >= 170000

Understood! I've refactored the code to include some JL_LLVM_VERSION guards.

src/Makefile Outdated Show resolved Hide resolved
src/aotcompile.cpp Outdated Show resolved Hide resolved
@giordano
Copy link
Contributor

giordano commented Apr 23, 2024

Good news: #54113 did fix a large chunk of failing packages, now we're down from 103 to 40 failing tests, only 17 of which started failing on this PR.

Most common error now seems to be (e.g. PreallocationTools.jl)

julia: /workspace/srcdir/llvm-project/llvm/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From*) [with To = llvm::IntegerType; From = llvm::Type]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

[887] signal 6 (-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/PreallocationTools/JFu7v/test/core_odes.jl:18
unknown function (ip: 0x7fb140cb7d3c)
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7fb140c53394)
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
_ZN4llvm15ScalarEvolution15willNotOverflowENS_11Instruction9BinaryOpsEbPKNS_4SCEVES5_PKS1_ at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN12_GLOBAL__N_119InductiveRangeCheck26extractRangeChecksFromCondEPN4llvm4LoopERNS1_15ScalarEvolutionERNS1_3UseERNS1_15SmallVectorImplIS0_EERNS1_15SmallPtrSetImplIPNS1_5ValueEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm8IRCEPass3runERNS_8FunctionERNS_15AnalysisManagerIS1_JEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
run at /source/usr/include/llvm/IR/PassManagerInternal.h:89
run at /source/usr/include/llvm/IR/PassManager.h:517 [inlined]
run at /source/usr/include/llvm/IR/PassManagerInternal.h:89
_ZN4llvm27ModuleToFunctionPassAdaptor3runERNS_6ModuleERNS_15AnalysisManagerIS1_JEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
run at /source/usr/include/llvm/IR/PassManagerInternal.h:89
_ZN4llvm11PassManagerINS_6ModuleENS_15AnalysisManagerIS1_JEEEJEE3runERS1_RS3_ at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
run at /source/src/pipeline.cpp:837
operator() at /source/src/jitlayers.cpp:1244
withModuleDo<(anonymous namespace)::OptimizerT<N>::operator()(llvm::orc::ThreadSafeModule, llvm::orc::MaterializationResponsibility&) [with long unsigned int N = 4]::<lambda(llvm::Module&)> > at /source/usr/include/llvm/ExecutionEngine/Orc/ThreadSafeModule.h:136 [inlined]
operator() at /source/src/jitlayers.cpp:1205 [inlined]
CallImpl<(anonymous namespace)::OptimizerT<4> > at /source/usr/include/llvm/ADT/FunctionExtras.h:220
_ZN4llvm3orc16IRTransformLayer4emitESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EENS0_16ThreadSafeModuleE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16IRTransformLayer4emitESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EENS0_16ThreadSafeModuleE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16IRTransformLayer4emitESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EENS0_16ThreadSafeModuleE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc31BasicIRLayerMaterializationUnit11materializeESt10unique_ptrINS0_29MaterializationResponsibilityESt14default_deleteIS3_EE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc19MaterializationTask3runEv at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm6detail18UniqueFunctionBaseIvJSt10unique_ptrINS_3orc4TaskESt14default_deleteIS4_EEEE8CallImplIPFvS7_EEEvPvRS7_ at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession22dispatchOutstandingMUsEv at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession17OL_completeLookupESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EESt10shared_ptrINS0_23AsynchronousSymbolQueryEESt8functionIFvRKNS_8DenseMapIPNS0_8JITDylibENS_8DenseSetINS0_15SymbolStringPtrENS_12DenseMapInfoISF_vEEEENSG_ISD_vEENS_6detail12DenseMapPairISD_SI_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc25InProgressFullLookupState8completeESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession19OL_applyQueryPhase1ESt10unique_ptrINS0_21InProgressLookupStateESt14default_deleteIS3_EENS_5ErrorE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupENS0_10LookupKindERKSt6vectorISt4pairIPNS0_8JITDylibENS0_19JITDylibLookupFlagsEESaIS8_EENS0_15SymbolLookupSetENS0_11SymbolStateENS_15unique_functionIFvNS_8ExpectedINS_8DenseMapINS0_15SymbolStringPtrENS0_17ExecutorSymbolDefENS_12DenseMapInfoISI_vEENS_6detail12DenseMapPairISI_SJ_EEEEEEEEESt8functionIFvRKNSH_IS6_NS_8DenseSetISI_SL_EENSK_IS6_vEENSN_IS6_SV_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
_ZN4llvm3orc16ExecutionSession6lookupERKSt6vectorISt4pairIPNS0_8JITDylibENS0_19JITDylibLookupFlagsEESaIS7_EENS0_15SymbolLookupSetENS0_10LookupKindENS0_11SymbolStateESt8functionIFvRKNS_8DenseMapIS5_NS_8DenseSetINS0_15SymbolStringPtrENS_12DenseMapInfoISI_vEEEENSJ_IS5_vEENS_6detail12DenseMapPairIS5_SL_EEEEEE at /opt/julia/bin/../lib/julia/libLLVM-17jl.so (unknown line)
addModule at /source/src/jitlayers.cpp:1868
jl_add_to_ee at /source/src/jitlayers.cpp:2359
jl_add_to_ee at /source/src/jitlayers.cpp:2338
jl_add_to_ee at /source/src/jitlayers.cpp:2338
jl_add_to_ee at /source/src/jitlayers.cpp:2338
jl_add_to_ee at /source/src/jitlayers.cpp:2338
jl_add_to_ee at /source/src/jitlayers.cpp:2338
jl_add_to_ee at /source/src/jitlayers.cpp:2338
[...]

Anyone got a clue? 😁

@gbaraldi
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@Zentrik
Copy link
Member

Zentrik commented Apr 23, 2024

I think you missed the first line of the error

julia: /workspace/srcdir/llvm-project/llvm/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From*) [with To = llvm::IntegerType; From = llvm::Type]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

@nanosoldier
Copy link
Collaborator

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

@gbaraldi
Copy link
Member

Performance looks good!

@gbaraldi
Copy link
Member

gbaraldi commented Apr 24, 2024

All the crashes Most of them seem to be LoopVectorization/ @chriselrod verse packages, which I think are known to not work. Though we might still want to figure out why it causes a crash.

@vchuravy
Copy link
Member

vchuravy commented Apr 24, 2024

StrBase

Seems to hit the same error in IRCE as most of the CR-verse packages do, but doesn't have a llvmcall warning

TidierDB
hits a sigabrt but no backtrace.

StringAlgorithms
Fails the IR verifier

@gbaraldi
Copy link
Member

gbaraldi commented Apr 24, 2024

The string algorithms one is a known LLVM bug sigh. It's llvm/llvm-project#63984 #50453

@gbaraldi
Copy link
Member

gbaraldi commented Apr 24, 2024

StrBase minimization ;)

function containsnul(d)
        b = Base.bitcast(Ptr{UInt}, getfield(d, :ptr))
        c, e = b,b
        while (c += 8) <= e
             f  end
end

containsnul(Memory{UInt}(undef,1))

@chriselrod
Copy link
Contributor

chriselrod commented Apr 24, 2024

I wouldn't worry about them too much

Maybe knowing what the issue is in particular, e.g. is it the opaque pointer change, might be worth knowing.

@gbaraldi
Copy link
Member

gbaraldi commented Apr 24, 2024

Unfortunately/Fortunately this seems to be an LLVM bug

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:10:11:12:13"
target triple = "x86_64-unknown-linux-gnu"

define void @julia_containsnul_1() {
top:
  br label %L3

L3:                                               ; preds = %L13, %top
  %value_phi = phi ptr [ null, %top ], [ %0, %L13 ]
  %0 = getelementptr i8, ptr %value_phi, i64 8
  %.not = icmp ule ptr %value_phi, null
  br i1 %.not, label %L13, label %L15

L13:                                              ; preds = %L3
  br label %L3

L15:                                              ; preds = %L3
  ret void
}

./opt --passes=irce test.ll -S to reproduce.

@gbaraldi
Copy link
Member

Opened issue upstream llvm/llvm-project#89959

@chriselrod
Copy link
Contributor

@gbaraldi Is that a minimal reproducer from LoopVectorization.jl?
Maybe I'm one of the few people using pointers as loop induction variables, if the problem was only uncovered now that Julia master represents its pointers as ptr instead of i$(8sizeof(Int)).
In certain situations (depending on addressing mode, e.g. x86's), it can improve codegen and reduce the amount of instructions needed in certain circumstances. LV actually has a lot of code dedicated to identifying those circumstances.

@gbaraldi
Copy link
Member

That is from StrBase, not sure if the patch will fix the LV issue

@gbaraldi
Copy link
Member

@nanosoldier runtests(["StringAlgorithms", "TriangularSolve", "Pyehtim", "GPUCompiler", "SurveyDataWeighting", "TidierDB", "StrBase", "Jadex", "TropicalGEMM", "BenchmarkProfiles", "LabelledArrays", "RandomFeatures", "FinEtools", "PreallocationTools", "BLASBenchmarksCPU", "Ai4EComponentLib", "LibTrixi"])

src/aotcompile.cpp Show resolved Hide resolved
src/jitlayers.cpp Show resolved Hide resolved
stdlib/LLD_jll/Project.toml Show resolved Hide resolved
test/llvmpasses/llvmcall.jl Show resolved Hide resolved
@nanosoldier
Copy link
Collaborator

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

@gbaraldi
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@gbaraldi
Copy link
Member

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

@nanosoldier
Copy link
Collaborator

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

@vchuravy vchuravy merged commit 1959349 into JuliaLang:master May 1, 2024
9 checks passed
vtjnash referenced this pull request May 3, 2024
Ensure that when an AnnotatedIOBuffer is wrapped in an IOContext (or
similar AnnotatedPipe-based construct), that writes of annotated
strings/chars and reading out an AnnotatedString is unimpeded by the
IOContext wrapping.

Without these specialisations, the generic pipe_reader/pipe_writer
fallbacks will directly access the underlying IOBuffer and annotations
will be lost.

There are a number of scenarios in which one might want to combine an
AnnotatedIOBuffer and IOContext (for example setting the compact
property). Losing annotations in such scenarios is highly undesirable.
It is of particular note that this can arise in situations where you
can't unwrap the IOContext as needed, for example when passing IO to a
function you do not control (which is currently extremely hard to work
around).

Getting this right is a little difficult, and a few approaches have been
tried. Initially, I added IOContext{AnnotatedIOBuffer} specialisations
to show.jl, but arguably it's a bit of a code smell to specialise in
this way (and Jameson wasn't happy with it, with concerns that it could
be tricked by IOContext{Any}).

    # So that read/writes with `IOContext` (and any similar `AbstractPipe` wrappers)
    # work as expected.
    write(io::IOContext{AnnotatedIOBuffer}, s::Union{AnnotatedString, SubString{<:AnnotatedString}}) =
        write(io.io, s)
    write(io::AnnotatedIOBuffer, c::AnnotatedChar) = write(io.io, c)

Then I tried making it so that IOContext writes dispatched on the
wrapped IO type, but of course that broke cases like IOContext{IOBuffer}
with :color=>true.

    # So that read/writes with `IOContext` (and any similar `AbstractPipe` wrappers)
    # work as expected.
    write(io::AbstractPipe, s::Union{AnnotatedString, SubString{<:AnnotatedString}}) =
        write(pipe_writer(io), s)
    write(io::AbstractPipe, c::AnnotatedChar) = write(pipe_writer(io), c)

Finally, we have the current AbstractPipe + Annotated type
specialisation, which IOContext is just an instance of. To avoid
behaving too broadly, we need to confirm that the underlying IO is
actually an AnnotatedIOBuffer. I'm still not happy with this, only idea
I've had other than implementing IOContext{AnnotatedIOBuffer} methods
that actually seems viable, and I've had trouble soliciting help from
other people brainstorming here.

If somebody can implement something cleaner here in the future, I'd be
thrilled.
@mofeing mofeing deleted the bump-llvm17 branch June 5, 2024 08:05
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.