Skip to content

Add support for LLVM 20 (modulo dynamic-compile regressions) #4911

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

Merged
merged 24 commits into from
May 12, 2025

Conversation

kinke
Copy link
Member

@kinke kinke commented Apr 19, 2025

Includes #4843.

kinke added 2 commits April 19, 2025 07:44
Based on checking the `ldc2 -help` output when using LLVM 20 and 19
on Linux x86_64 (with all targets enabled, incl. experimental SPIRV/
Xtensa). And comparing that against official LDC v1.34 with LLVM 16.
@kinke
Copy link
Member Author

kinke commented Apr 19, 2025

There's a 32-bit x86 optimizer (?) regression blocking more CI results for the Win32 and Linux multilib CI jobs. This hits an LLVM 20 assertion:

$ bin/ldc2 -c -unittest ../ldc/runtime/druntime/src/core/internal/container/array.d -d-version=CoreUnittest -m32 -release -O
ldc2: /home/martin/dev/llvm-project/llvm/include/llvm/ADT/APInt.h:127: llvm::APInt::APInt(unsigned int, uint64_t, bool, bool): Assertion `llvm::isUIntN(BitWidth, val) && "Value is not an N-bit unsigned value"' failed.
[…]
#11 0x000064fa30bbae89 llvm::maxIntN(long) /home/martin/dev/llvm-project/llvm/include/llvm/Support/MathExtras.h:252:35
#12 0x000064fa30bbae89 llvm::isIntN(unsigned int, long) /home/martin/dev/llvm-project/llvm/include/llvm/Support/MathExtras.h:262:53
#13 0x000064fa30bbae89 llvm::APInt::APInt(unsigned int, unsigned long, bool, bool) /home/martin/dev/llvm-project/llvm/include/llvm/ADT/APInt.h:120:11
#14 0x000064fa3487eb66 llvm::BasicAAResult::aliasGEP(llvm::GEPOperator const*, llvm::LocationSize, llvm::Value const*, llvm::LocationSize, llvm::Value const*, llvm::Value const*, llvm::AAQueryInfo&) /home/martin/dev/llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp:1308:21
#15 0x000064fa3487fa4f llvm::BasicAAResult::aliasCheckRecursive(llvm::Value const*, llvm::LocationSize, llvm::Value const*, llvm::LocationSize, llvm::AAQueryInfo&, llvm::Value const*, llvm::Value const*) /home/martin/dev/llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp:1777:5
#16 0x000064fa34881e0f llvm::BasicAAResult::aliasCheck(llvm::Value const*, llvm::LocationSize, llvm::Value const*, llvm::LocationSize, llvm::AAQueryInfo&, llvm::Instruction const*) /home/martin/dev/llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp:1722:33
#17 0x000064fa3488255c llvm::BasicAAResult::alias(llvm::MemoryLocation const&, llvm::MemoryLocation const&, llvm::AAQueryInfo&, llvm::Instruction const*) /home/martin/dev/llvm-project/llvm/lib/Analysis/BasicAliasAnalysis.cpp:890:73
#18 0x000064fa34858ef2 llvm::AAResults::alias(llvm::MemoryLocation const&, llvm::MemoryLocation const&, llvm::AAQueryInfo&, llvm::Instruction const*) /home/martin/dev/llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp:125:23
#19 0x000064fa3485af28 llvm::AAResults::getModRefInfo(llvm::LoadInst const*, llvm::MemoryLocation const&, llvm::AAQueryInfo&) /home/martin/dev/llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp:441:5
#20 0x000064fa3485af28 llvm::AAResults::getModRefInfo(llvm::Instruction const*, std::optional<llvm::MemoryLocation> const&, llvm::AAQueryInfo&) /home/martin/dev/llvm-project/llvm/lib/Analysis/AliasAnalysis.cpp:580:25
#21 0x000064fa3422da4b llvm::SimpleCaptureAnalysis::~SimpleCaptureAnalysis() /home/martin/dev/llvm-project/llvm/include/llvm/Analysis/AliasAnalysis.h:162:7
#22 0x000064fa3422da4b llvm::SimpleAAQueryInfo::~SimpleAAQueryInfo() /home/martin/dev/llvm-project/llvm/include/llvm/Analysis/AliasAnalysis.h:305:7
[…]

Edit: Seems to happen for all 32-bit targets, at least for Android ARMv7-A and riscv32.

@liushuyu
Copy link
Contributor

I can take a look.
Sorry, I was busy with a lot of IRL stuff recently and was unable to do anything else.

@kinke
Copy link
Member Author

kinke commented Apr 21, 2025

No worries! - Yeah it'd be great if you could look into that problem.

@kinke kinke force-pushed the llvm-20_2 branch 2 times, most recently from ef25b12 to 737d77d Compare April 26, 2025 16:12
kinke added 2 commits April 26, 2025 18:42
Where the ASan pass now apparently adds a `nosanitize_address` IR module
flag, breaking these tests.
@kinke
Copy link
Member Author

kinke commented Apr 26, 2025

The same LLVM assertion is also hit for lit-test dynamiccompile/bind_bool.d, on macOS (where it's the single remaining failure for both x86_64 and arm64 archs) and Linux aarch64.

Looks like the other remaining failures on non-macOS are all dynamic-compile related - 4 hanging lit-tests on Windows x64 (incl. bind_bool.d), and 28 failures on Linux aarch64 (seemingly getting a signal 2 - interrupt).

@kinke kinke changed the title CI: Bump LDC-LLVM to v20.1.3 CI: Bump LDC-LLVM to v20.1.4 May 2, 2025
@liushuyu
Copy link
Contributor

liushuyu commented May 4, 2025

I only managed to take a look at the assertion issue this afternoon. The minimal reproducer (in LLVM IR) looks like this:

source_filename = "reduced.ll"
target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-i128:128-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-unknown-linux-gnu"

%"core.internal.container.common.RC!().RC" = type { ptr }

; Function Attrs: nofree norecurse nosync nounwind memory(write, argmem: readwrite, inaccessiblemem: none)
define x86_stdcallcc void @_D4core8internal9container5array__T5ArrayTSQBpQBnQBh6common__T2RCZQeZQBi__T10insertBackZQnMFNbNiQCcZv(ptr nocapture %0) local_unnamed_addr #0 personality ptr null {
if:
  br label %forbody.i

forbody.i:                                        ; preds = %forbody.i, %if
  %__key37.01.i = phi i32 [ %3, %forbody.i ], [ 0, %if ]
  %1 = getelementptr %"core.internal.container.common.RC!().RC", ptr %0, i32 %__key37.01.i
  %2 = load ptr, ptr %1, align 4
  store i32 0, ptr %2, align 4
  store i32 0, ptr %1, align 1
  %3 = add i32 %__key37.01.i, 1
  %exitcond.not.i = icmp eq i32 %__key37.01.i, -1
  br i1 %exitcond.not.i, label %_D4core8internal9container5array__T5ArrayTSQBpQBnQBh6common__T2RCZQeZQBi6lengthMFNbNdNikZv.exit, label %forbody.i

_D4core8internal9container5array__T5ArrayTSQBpQBnQBh6common__T2RCZQeZQBi6lengthMFNbNdNikZv.exit: ; preds = %forbody.i
  ret void
}

attributes #0 = { nofree norecurse nosync nounwind memory(write, argmem: readwrite, inaccessiblemem: none) }

It seems like LDC will blow up LLVM if the code contains patterns like this (needs -m32 switch to trigger):

struct RC
{
    int* ptr;
}

void repro(RC* arg0)
{
    for (int i = 0; i != -1; i++)
    {
        int* ptr = arg0[i].ptr;
        *ptr = 0;
        arg0[i].ptr = cast(int*)0;
    }
}

I will investigate further if the issue is with LDC or LLVM.

@kinke
Copy link
Member Author

kinke commented May 4, 2025

Thx for digging! - It looks like we don't need a struct for this, nor a signed index/offset. This still hits the assertion with -O -m32:

void repro(ubyte** arg0)
{
    for (size_t i = 0; i != int.max / 2 + 1; i++)
    {
        *arg0[i] = 0;
        arg0[i] = null;
    }
}

i != int.max / 2 works, and i != int.max / 2 - 1 too.

Edit: Ah okay, the problematic max index seems to be size_t.max / ubyte*.sizeof, where the GEP would be about to wrap. For 32-bit at least; I can't get it to fail for 64-bit. And IIRC, we emit ~all GEPs as inbounds.

Do we really have testcases with such a huge static index range? As soon as I add a break depending on the current loop value, the assertion isn't hit anymore.

@kinke
Copy link
Member Author

kinke commented May 4, 2025

Do we really have testcases with such a huge static index range?

Oh I see:

unittest
{
import core.exception;
try
{
// Overflow ary.length.
auto ary = Array!size_t(cast(size_t*)0xdeadbeef, -1);
ary.insertBack(0);
}
catch (OutOfMemoryError)
{
}
try
{
// Overflow requested memory size for common.xrealloc().
auto ary = Array!size_t(cast(size_t*)0xdeadbeef, -2);
ary.insertBack(0);
}
catch (OutOfMemoryError)
{
}
}

Edit: Nope, commenting that out doesn't suffice.

@liushuyu
Copy link
Contributor

liushuyu commented May 4, 2025

Edit: Ah okay, the problematic max index seems to be size_t.max / ubyte*.sizeof, where the GEP would be about to wrap. For 32-bit at least; I can't get it to fail for 64-bit. And IIRC, we emit ~all GEPs as inbounds.

I don't think inbounds is relevant. LLVM optimizer tried to do alias analysis on this GEP and failed to create the analysis pipeline due to too large of an (emulated) address. This seems to be a regression since LLVM 19.

@liushuyu
Copy link
Contributor

liushuyu commented May 5, 2025

Ah, it seems like someone else encountered the same issue here: llvm/llvm-project#119365 (comment)

@liushuyu
Copy link
Contributor

liushuyu commented May 5, 2025

@kinke A fix is being prepared in llvm/llvm-project#138528. You might want to backport this patch to the LDC fork of LLVM.

Also, the fix (in git mailbox format) for the JIT issue (I don't think I should directly write to your repository):

From 1dfedc20a40da04a8c1e476871f14d5869ec0e1c Mon Sep 17 00:00:00 2001
From: liushuyu <liushuyu011@gmail.com>
Date: Mon, 5 May 2025 23:46:40 +0800
Subject: [PATCH] jit-rt: avoid double registering EHFrame plugin ...

... when using LLVM JITLink on LLVM 20+
---
 runtime/jit-rt/cpp-so/jit_context.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/runtime/jit-rt/cpp-so/jit_context.cpp b/runtime/jit-rt/cpp-so/jit_context.cpp
index 88dcac4d0e..ccbd9011ec 100644
--- a/runtime/jit-rt/cpp-so/jit_context.cpp
+++ b/runtime/jit-rt/cpp-so/jit_context.cpp
@@ -113,7 +113,9 @@ static llvm::orc::LLJITBuilder buildLLJITforLDC() {
   // we override the object linking layer if we are using LLVM JITLink.
   // For RuntimeDyld, we use LLJIT's default setup process
   // (which includes a lot of platform-related workarounds we need)
-#ifdef LDC_JITRT_USE_JITLINK
+  // on LLVM 20+, LLJIT will auto-configure eh-frame plugin and
+  // we avoid configuring the eh-frame plugin ourselves to avoid double registration
+#if defined(LDC_JITRT_USE_JITLINK) && LDC_LLVM_VER < 2000
       .setObjectLinkingLayerCreator([&](llvm::orc::ExecutionSession &ES,
                                         const llvm::Triple &TT) {
         auto linker = std::make_unique<llvm::orc::ObjectLinkingLayer>(
-- 
2.49.0

@kinke
Copy link
Member Author

kinke commented May 5, 2025

Awesome, thx a lot mate!

... when using LLVM JITLink on LLVM 20+
@kinke kinke changed the title CI: Bump LDC-LLVM to v20.1.4 CI: Bump LDC-LLVM to v20.1.5 May 10, 2025
@kinke
Copy link
Member Author

kinke commented May 10, 2025

Okay, only some dynamic-compile failures remaining - 1 APInt assertion on macOS, 4 hangs on Windows, 10 failures on Linux.

@liushuyu
Copy link
Contributor

Okay, only some dynamic-compile failures remaining - 1 APInt assertion on macOS, 4 hangs on Windows, 10 failures on Linux.

It seems to me that there might be some other LLVM JIT misuses or LLVM regressions.

@kinke kinke force-pushed the llvm-20_2 branch 2 times, most recently from 21477c6 to 71fd203 Compare May 10, 2025 14:33
@kinke
Copy link
Member Author

kinke commented May 10, 2025

I could switch CI back to LDC-LLVM 19 for now, making this an 'add LLVM 20 support modulo dynamic-compile' PR that we could merge earlier. You could then simply revert that commit in a rebased #4924 to run CI with LLVM 20.

@liushuyu
Copy link
Contributor

I could switch CI back to LDC-LLVM 19 for now, making this an 'add LLVM 20 support modulo dynamic-compile' PR that we could merge earlier. You could then simply revert that commit in a rebased #4924 to run CI with LLVM 20.

Sounds good. Let's do it this way then.
I will investigate the problem and prepare the fix in a different pull request.

@kinke kinke changed the title CI: Bump LDC-LLVM to v20.1.5 Add support for LLVM 20 (modulo dynamic-compile regressions) May 10, 2025
@kinke kinke marked this pull request as ready for review May 11, 2025 00:15
@kinke kinke merged commit 53ffdf1 into ldc-developers:master May 12, 2025
19 of 20 checks passed
@kinke kinke deleted the llvm-20_2 branch May 12, 2025 18:25
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.

2 participants