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 v19.1.1+1 #56130

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Bump LLVM to v19.1.1+1 #56130

wants to merge 11 commits into from

Conversation

Zentrik
Copy link
Member

@Zentrik Zentrik commented Oct 12, 2024

Including #55650 till that's merged.

@Zentrik Zentrik added compiler:llvm For issues that relate to LLVM building Build system, or building Julia or its dependencies external dependencies Involves LLVM, OpenBLAS, or other linked libraries JLLs labels Oct 12, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Oct 12, 2024

The /home/rag/Documents/Code/julia/usr/include/llvm/ADT/StringRef.h:871:20: warning: 'int __builtin_memcmp_eq(const void*, const void*, long unsigned int)' specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overread] warning seems to be a gcc bug, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101361

EDIT: gcc is giving a slightly different warning on CI but likely a bug as well

@giordano
Copy link
Contributor

In that case you can guard the offending line with something like

#pragma GCC diagnostic push
#if defined(_COMPILER_GCC_) && __GNUC__ >= 12 // if this is version-dependent
// Explain why this is being ignored...
#pragma GCC diagnostic ignored "-Wstringop-overflow"
#endif
...
#pragma GCC diagnostic pop

@giordano
Copy link
Contributor

Also, all tests are passing already on aarch64-darwin 🥳

@Zentrik
Copy link
Member Author

Zentrik commented Oct 12, 2024

Looks like the only two issues are a incorrect warning during the build (mose's suggestion doesn't seem to work though #pragma GCC diagnostic ignored "-Wstringop-overflow" by itself does) and /cache/build/tester-demeter6-13/julialang/julia-master/tmp/test-asan/asan/usr/bin/julia: error while loading shared libraries: libclang_rt.asan-x86_64.so: cannot open shared object file: No such file or directory. (I'll push the analyzegc fix in a bit).

@Zentrik
Copy link
Member Author

Zentrik commented Oct 12, 2024

Guessing the asan problem is clang -print-runtime-dir is returning toolchain/usr/lib/clang/19/lib/x86_64-unknown-linux-gnu instead of toolchain/usr/lib/clang/19/lib/linux so we don't find libclang_rt.asan... to copy to the build folder.

@giordano
Copy link
Contributor

though #pragma GCC diagnostic ignored "-Wstringop-overflow" by itself does

Without the push-and-pop, that becomes closer to just adding -Wstringop-overflow to

julia/src/Makefile

Lines 23 to 25 in 80e60c8

ifeq ($(USEGCC),1) # GCC bug #25509 (void)__attribute__((warn_unused_result))
FLAGS += -Wno-unused-result
endif
(please always add a comment to explain why the warning is being skipped) but it'd be nicer to keep the ignore as local as possible, if feasible.

@Zentrik Zentrik force-pushed the llvm-19-actual branch 2 times, most recently from 5c3c307 to 24f4d93 Compare October 13, 2024 09:48
@Zentrik
Copy link
Member Author

Zentrik commented Oct 13, 2024

Looks like remaining issue is some failures in analyzegc that I guess weren't caught by previous versions of clang.

@giordano giordano added the needs pkgeval Tests for all registered packages should be run with this change label Oct 13, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Oct 13, 2024

Second analyzegc failure looks incorrect, it seems to incorrectly think uv_dup could return 0 when dupfd is set to -1 (minimal reproducer https://godbolt.org/z/4Wo99v1nv, llvm/llvm-project#43015 looks to be the same issue).

@Zentrik
Copy link
Member Author

Zentrik commented Oct 13, 2024

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

@giordano
Copy link
Contributor

Need to rebase on master now that #56133 has been merged.

@Zentrik Zentrik force-pushed the llvm-19-actual branch 2 times, most recently from f6b2376 to e12026f Compare October 13, 2024 20:43
@nanosoldier
Copy link
Collaborator

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

@vtjnash
Copy link
Member

vtjnash commented Oct 14, 2024

First analyzegc failure looks incorrect, it seems to incorrectly think uv_dup could return 0 when dupfd is set to -1 (minimal reproducer https://godbolt.org/z/4Wo99v1nv, llvm/llvm-project#43015 looks to be the same issue).

FWIW, the standard way to deal with this is to add an assert and a comment, as that will make both reviewers and bots happy

@Zentrik
Copy link
Member Author

Zentrik commented Oct 16, 2024

The union.array.(perf_sum, Float32, 0) regression is due to SIMD not occurring. This is due to a change in the result from the instcombine pass. See https://godbolt.org/z/qqdzM7oxh,
The change from

%12 = select i1 %exactly_isa.not, float -0.000000e+00, float %immutable_union.sroa.0.0.copyload, !dbg !77
%value_phi6 = fadd float %value_phi, %12, !dbg !77

to

%12 = fadd float %value_phi, %immutable_union.sroa.0.0.copyload, !dbg !77
%value_phi6 = select i1 %exactly_isa.not, float %value_phi, float %12, !dbg !77

prevents the loopvectorize pass from SIMDing the code.

I suspect the other regression for the union.array benchmarks are similar.

@Zentrik
Copy link
Member Author

Zentrik commented Oct 17, 2024

I'll try rebasing on top of #52850 and see if that fixes the regressions.

@Zentrik
Copy link
Member Author

Zentrik commented Oct 17, 2024

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

@oscardssmith
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":gb/pipeline-fun")

@nanosoldier
Copy link
Collaborator

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

@oscardssmith
Copy link
Member

oscardssmith commented Oct 19, 2024

looks like there are a few 8x (vectorization probably) regressions here.

@Zentrik
Copy link
Member Author

Zentrik commented Oct 27, 2024

PkgEval failure for McCormick is likely llvm/llvm-project#113810 llvm/llvm-project#101213, fixed by llvm/llvm-project@f70f122.

@giordano
Copy link
Contributor

giordano commented Nov 3, 2024

Analyzegc is still failing:

/cache/build/builder-demeter6-5/julialang/julia-master/usr/include/llvm/ADT/APInt.h:172:7: error: Attempt to free released memory [cplusplus.NewDelete]
  172 |       delete[] U.pVal;
      |       ^

`clang -print-runtime-dir` reports a non-existent directory as we build with `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF`. See llvm/llvm-project#102834. I suspect llvm/llvm-project@b6a1473 caused the change by chaning the code in Driver.cpp to not check whether the printed directory existed.
Locally I get:
```
In file included from /home/rag/Documents/Code/julia/usr/include/llvm/Object/ObjectFile.h:18,
                 from /home/rag/Documents/Code/julia/usr/include/llvm/DebugInfo/DIContext.h:18,
                 from /home/rag/Documents/Code/julia/src/debuginfo.cpp:6:
In function 'bool llvm::operator==(llvm::StringRef, llvm::StringRef)',
    inlined from 'bool llvm::operator!=(llvm::StringRef, llvm::StringRef)' at /home/rag/Documents/Code/julia/usr/include/llvm/ADT/StringRef.h:874:71,
    inlined from 'objfileentry_t find_object_file(uint64_t, llvm::StringRef)' at /home/rag/Documents/Code/julia/src/debuginfo.cpp:948:43,
    inlined from 'bool jl_dylib_DI_for_fptr(size_t, llvm::object::SectionRef*, int64_t*, llvm::DIContext**, bool, bool*, uint64_t*, void**, char**, char**)' at /home/rag/Documents/Code/julia/src/debuginfo.cpp:1135:34:
/home/rag/Documents/Code/julia/usr/include/llvm/ADT/StringRef.h:871:20: warning: 'int __builtin_memcmp_eq(const void*, const void*, long unsigned int)' specified bound 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Wstringop-overread]
  871 |     return ::memcmp(LHS.data(), RHS.data(), LHS.size()) == 0;
      |            ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/rag/Documents/Code/julia/src/debuginfo.cpp: In function 'bool jl_dylib_DI_for_fptr(size_t, llvm::object::SectionRef*, int64_t*, llvm::DIContext**, bool, bool*, uint64_t*, void**, char**, char**)':
/home/rag/Documents/Code/julia/src/debuginfo.cpp:1133:11: note: source object allocated here
 1133 |     fname = dlinfo.dli_fname;
 ```

 On CI:
```
In file included from /cache/build/builder-amdci4-2/julialang/julia-master/usr/include/llvm/Object/ObjectFile.h:18,
                 from /cache/build/builder-amdci4-2/julialang/julia-master/usr/include/llvm/DebugInfo/DIContext.h:18,
                 from /cache/build/builder-amdci4-2/julialang/julia-master/src/debuginfo.cpp:6:
In function 'bool llvm::operator==(llvm::StringRef, llvm::StringRef)',
    inlined from 'bool llvm::operator!=(llvm::StringRef, llvm::StringRef)' at /cache/build/builder-amdci4-2/julialang/julia-master/usr/include/llvm/ADT/StringRef.h:874:71,
    inlined from 'objfileentry_t find_object_file(uint64_t, llvm::StringRef)' at /cache/build/builder-amdci4-2/julialang/julia-master/src/debuginfo.cpp:943:43,
    inlined from 'bool jl_dylib_DI_for_fptr(size_t, llvm::object::SectionRef*, int64_t*, llvm::DIContext**, bool, bool*, uint64_t*, void**, char**, char**)' at /cache/build/builder-amdci4-2/julialang/julia-master/src/debuginfo.cpp:1126:47:
/cache/build/builder-amdci4-2/julialang/julia-master/usr/include/llvm/ADT/StringRef.h:871:20: error: 'int __builtin_memcmp_eq(const void*, const void*, long unsigned int)' specified size 18446744073709551615 exceeds maximum object size 9223372036854775807 [-Werror=stringop-overflow=]
  871 |     return ::memcmp(LHS.data(), RHS.data(), LHS.size()) == 0;
      |            ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
The change to `GCChecker.cpp` is so the static analyzer doesn't see e.g. a call to a function pointer in llvm and then complain that it might be a safepoint.
e7698a13e319a9919af04d3d693a6f6ea7168a44 isn't in llvm 19
@Zentrik Zentrik changed the title Bump LLVM to v19.1.1+0 Bump LLVM to v19.1.1+1 Nov 24, 2024
@Zentrik
Copy link
Member Author

Zentrik commented Nov 24, 2024

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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

@giordano
Copy link
Contributor

giordano commented Nov 25, 2024

On this PR code_native(...; debuginfo=:none) doesn't hide some debug info:

julia> code_llvm(+, NTuple{2,Float64}; debuginfo=:none)
; Function Signature: +(Float64, Float64)
define double @"julia_+_1011"(double %"x::Float64", double %"y::Float64") #0 {
top:
    #dbg_value(double %"x::Float64", !3, !DIExpression(), !16)
    #dbg_value(double %"y::Float64", !15, !DIExpression(), !16)
  %0 = fadd double %"x::Float64", %"y::Float64"
  ret double %0
}

Looks like the syntax of the debug values was changed in LLVM 19.

@giordano
Copy link
Contributor

giordano commented Dec 2, 2024

Other issue related to native/llvm code printing. On master, with LLVM 18:

julia> code_native(+, NTuple{2,Float64}; debuginfo=:none)
        .text
        .file   "+"
        .section        .ltext,"axl",@progbits
        .globl  "julia_+_738"                   # -- Begin function julia_+_738
        .p2align        4, 0x90
        .type   "julia_+_738",@function
"julia_+_738":                          # @"julia_+_738"
; Function Signature: +(Float64, Float64)
# %bb.0:                                # %top
        #DEBUG_VALUE: +:x <- $xmm0
        #DEBUG_VALUE: +:y <- $xmm1
        push    rbp
        mov     rbp, rsp
        vaddsd  xmm0, xmm0, xmm1
        pop     rbp
        ret
.Lfunc_end0:
        .size   "julia_+_738", .Lfunc_end0-"julia_+_738"
                                        # -- End function
        .type   ".L+Core.Float64#740",@object   # @"+Core.Float64#740"
        .section        .lrodata,"al",@progbits
        .p2align        3, 0x0
".L+Core.Float64#740":
        .quad   ".L+Core.Float64#740.jit"
        .size   ".L+Core.Float64#740", 8

.set ".L+Core.Float64#740.jit", 139704953807808
        .size   ".L+Core.Float64#740.jit", 8
        .section        ".note.GNU-stack","",@progbits

On this PR:

julia> code_native(+, NTuple{2,Float64}; debuginfo=:none)
        .text
        .file   "+"
        .section        .ltext,"axl",@progbits
        .globl  "julia_+_682"
        .p2align        4, 0x90
        .type   "julia_+_682",@function
"julia_+_682":
        push    rbp
        mov     rbp, rsp
        vaddsd  xmm0, xmm0, xmm1
        pop     rbp
        ret
.Lfunc_end0:
        .size   "julia_+_682", .Lfunc_end0-"julia_+_682"

        .type   ".L+Core.Float64#684",@object
        .section        .lrodata,"al",@progbits
        .p2align        3, 0x0
".L+Core.Float64#684":
        .quad   ".L+Core.Float64#684.jit"
        .size   ".L+Core.Float64#684", 8

.set ".L+Core.Float64#684.jit", 140587410418304
        .size   ".L+Core.Float64#684.jit", 8
        .section        ".note.GNU-stack","",@progbits

Note that all the comments are gone. This makes reading complex code harder. I don't know if something changed in LLVM API to ask for the comments.

@giordano
Copy link
Contributor

giordano commented Dec 2, 2024

Another issue is that SVE vectorisation on aarch64 seems to be broken (in the sense that it isn't used). Consider for example the function

function axpy!(a, x, y)
    @simd for idx in eachindex(x, y)
        @inbounds y[idx] = muladd(a, x[idx], y[idx])
    end
end

With code_llvm(axpy!, (Float64, Vector{Float64}, Vector{Float64}); debuginfo=:none) or code_native(axpy!, (Float64, Vector{Float64}, Vector{Float64}); debuginfo=:none) one case see that SVE registries aren't used anymore, but only NEON ones. Performance isn't necessarily worse for same register width, but SVE has (potentially) larger width than NEON and is in general more capable (it supports more datatypes).

@vtjnash
Copy link
Member

vtjnash commented Dec 2, 2024

IIUC, someone deleted support for printing comments on our end:

            std::unique_ptr<MCStreamer> S(TM->getTarget().createAsmStreamer(
#if JL_LLVM_VERSION >= 190000
                *Context, std::move(FOut), InstPrinter, std::move(MCE), std::move(MAB)
#else
                *Context, std::move(FOut), /*AsmVerbose*/true, true, InstPrinter, std::move(MCE),
                std::move(MAB), false
#endif
                    ));

This is now supposed to be set here rather than deleted:
https://llvm.org/doxygen/classllvm_1_1MCTargetOptions.html#a192e9c2a63352ff1000ebe757e5b1f12

@vtjnash
Copy link
Member

vtjnash commented Dec 4, 2024

/usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/optional:236:18: note: Calling implicit destructor for 'ConstantRange'
236 | ~_Storage() { }

That is odd. The C++ standard says that this implicit destructor is not permitted to be called, so it is strange to see the analyzer thinking otherwise than what the standard permits.

@vtjnash
Copy link
Member

vtjnash commented Dec 4, 2024

MWE for the apparent clang-analyzer mistake:

vtjnash@deepsea4:~/julia$ clang++ -fPIE -std=c++20 dtor-test.cpp  && ./a.out                                                                                                                                       
C Ctor                                                                                                     
C Dtor                                                                                                     
vtjnash@deepsea4:~/julia$ g++ -fPIE -std=c++20 dtor-test.cpp  && ./a.out                                                                                                                                               
C Ctor                                                                                                                                                                                                                 
C Dtor                                                                                                     
$ cat dtor-test.cpp 
#include <iostream>
#include <optional>

class C {
  int *once;
public:
  C(int) {
    std::cout << "C Ctor" << std::endl;
    once = new int[3];
  }
  C(C&&) = delete;
  ~C() {
    std::cout << "C Dtor" << std::endl;
    delete[] once;
  }
};

int main() {
  std::optional<C> S{1};
  return 0;
}

$ ./usr/tools/clang++ --analyze -analyzer-output=text -std=c++20 dtor-test.cpp 
dtor-test.cpp:14:5: warning: Attempt to free released memory [cplusplus.NewDelete]                                                                                                                                     
   14 |     delete[] once;                                                                                 
      |     ^                                                                                                                                                                                                          

@vtjnash
Copy link
Member

vtjnash commented Dec 10, 2024

patch to add to fix this #56130 (comment)

diff --git a/src/disasm.cpp b/src/disasm.cpp
index b944e48430c..cc296618635 100644
--- a/src/disasm.cpp
+++ b/src/disasm.cpp
@@ -873,6 +873,8 @@ static void jl_dump_asm_internal(
     SourceMgr SrcMgr;
 
     MCTargetOptions Options;
+    Options.AsmVerbose = true;
+    Options.MCUseDwarfDirectory = MCTargetOptions::EnableDwarfDirectory;
     std::unique_ptr<MCAsmInfo> MAI(
         TheTarget->createMCAsmInfo(*TheTarget->createMCRegInfo(TheTriple.str()), TheTriple.str(), Options));
     assert(MAI && "Unable to create target asm info!");
@@ -1274,8 +1276,13 @@ jl_value_t *jl_dump_function_asm_impl(jl_llvmf_dump_t* dump, char emit_mc, const
                 OutputAsmDialect = 1;
             MCInstPrinter *InstPrinter = TM->getTarget().createMCInstPrinter(
                 jl_ExecutionEngine->getTargetTriple(), OutputAsmDialect, MAI, MII, MRI);
-             std::unique_ptr<MCAsmBackend> MAB(TM->getTarget().createMCAsmBackend(
-                STI, MRI, TM->Options.MCOptions));
+            MCTargetOptions Options = TM->Options.MCOptions;
+            Options.AsmVerbose = true;
+            Options.MCUseDwarfDirectory = MCTargetOptions::EnableDwarfDirectory;
+            if (binary)
+                Options.ShowMCEncoding = true;
+            std::unique_ptr<MCAsmBackend> MAB(TM->getTarget().createMCAsmBackend(
+                STI, MRI, Options));
             std::unique_ptr<MCCodeEmitter> MCE;
             if (binary) { // enable MCAsmStreamer::AddEncodingComment printing
                 MCE.reset(TM->getTarget().createMCCodeEmitter(MII, *Context));
@@ -1289,8 +1296,7 @@ jl_value_t *jl_dump_function_asm_impl(jl_llvmf_dump_t* dump, char emit_mc, const
                 std::move(MAB), false
 #endif
                     ));
-            std::unique_ptr<AsmPrinter> Printer(
-                TM->getTarget().createAsmPrinter(*TM, std::move(S)));
+            AsmPrinter *Printer = TM->getTarget().createAsmPrinter(*TM, std::move(S));
 #if JL_LLVM_VERSION >= 190000
             Printer->addDebugHandler(
                         std::make_unique<LineNumberPrinterHandler>(*Printer, debuginfo));
@@ -1301,7 +1307,7 @@ jl_value_t *jl_dump_function_asm_impl(jl_llvmf_dump_t* dump, char emit_mc, const
 #endif
             if (!Printer)
                 return jl_an_empty_string;
-            PM.add(Printer.release());
+            PM.add(Printer);
             PM.add(createFreeMachineFunctionPass());
             TSM->withModuleDo([&](Module &m){ PM.run(m); });
         }

Looks like the syntax of the debug values was changed in LLVM 19

The jl_strip_llvm_debug needs to handle the new syntax (add a call to dropDbgRecords)

diff --git a/src/disasm.cpp b/src/disasm.cpp
index c3488afc4d9..40f23e869da 100644
--- a/src/disasm.cpp
+++ b/src/disasm.cpp
@@ -460,6 +460,9 @@ static void jl_strip_llvm_debug(Module *m, bool all_meta, LineNumberAnnotatedWri
                 if (AAW)
                     AAW->addDebugLoc(&inst, inst.getDebugLoc());
                 inst.setDebugLoc(DebugLoc());
+#if JL_LLVM_VERSION >= 190000
+                inst.dropDbgRecords();
+#endif
             }
             if (deletelast) {
                 deletelast->eraseFromParent();

@Zentrik
Copy link
Member Author

Zentrik commented Dec 11, 2024

Thanks for the patch but for reasons I don't understand yet, the following doesn't seem to work as debug info is not printed

@@ -1274,8 +1276,13 @@ jl_value_t *jl_dump_function_asm_impl(jl_llvmf_dump_t* dump, char emit_mc, const
                 OutputAsmDialect = 1;
             MCInstPrinter *InstPrinter = TM->getTarget().createMCInstPrinter(
                 jl_ExecutionEngine->getTargetTriple(), OutputAsmDialect, MAI, MII, MRI);
-             std::unique_ptr<MCAsmBackend> MAB(TM->getTarget().createMCAsmBackend(
-                STI, MRI, TM->Options.MCOptions));
+            MCTargetOptions Options = TM->Options.MCOptions;
+            Options.AsmVerbose = true;
+            Options.MCUseDwarfDirectory = MCTargetOptions::EnableDwarfDirectory;
+            if (binary)
+                Options.ShowMCEncoding = true;
+            std::unique_ptr<MCAsmBackend> MAB(TM->getTarget().createMCAsmBackend(
+                STI, MRI, Options));
             std::unique_ptr<MCCodeEmitter> MCE;
             if (binary) { // enable MCAsmStreamer::AddEncodingComment printing
                 MCE.reset(TM->getTarget().createMCCodeEmitter(MII, *Context));

Instead if I do the following, the debug info is printed correctly

diff --git a/src/jitlayers.cpp b/src/jitlayers.cpp
index e5a96cdf9d..a1fea54a16 100644
--- a/src/jitlayers.cpp
+++ b/src/jitlayers.cpp
@@ -1381,6 +1381,8 @@ namespace {
         options.MCOptions.ABIName = "lp64";
 #endif
 #endif
+        options.MCOptions.AsmVerbose = true;
+        options.MCOptions.MCUseDwarfDirectory = MCTargetOptions::EnableDwarfDirectory;
         uint32_t target_flags = 0;
         auto target = jl_get_llvm_target(imaging_default(), target_flags);
         auto &TheCPU = target.first;

Also, the ; Function Signature: +(Float64, Float64) in missing in the output of code_native but not code_llvm for some other reason I haven't quite figured out.

@giordano
Copy link
Contributor

If that helps, the signature was added with #50630, to give an idea of where to look at in the code.

@Zentrik
Copy link
Member Author

Zentrik commented Dec 11, 2024

Thanks. On the function signature not printing it looks like we null out the printer here which prevents the printing
https://github.com/llvm/llvm-project/blob/9cdb7d2b6c333874ec969ef6ac64e0354bb3aa91/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp#L105-L108

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 needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants