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

LLVM10 changes #4409

Merged
merged 2 commits into from
Feb 7, 2020
Merged

LLVM10 changes #4409

merged 2 commits into from
Feb 7, 2020

Conversation

LemonBoy
Copy link
Contributor

@LemonBoy LemonBoy commented Feb 7, 2020

Nothing to see here, move along

  • Solves the mysterious lack of stack traces, turns out LLVM changed some tags a while ago and now the old ones the codegen used to use are silently ignored.
  • I've added some more stack-traces test, please adapt it to other platforms than linux too.
  • RISCV64-linux is now working quite well:
    • RISC-V behavior tests disabled #3338 can be closed
    • We can't use the baseline_rv64 CPU as-is since lld doesn't support some relocations/relaxations, the trick is to pass an additional -relax to the default feature set. The feature set API is so damn awkward to use that I had to give up after wrestling with it for a while.
    • The instruction selector chokes on this pattern (cc @luismarques)
Cannot select: 0x55d93f824958: i32,ch = AtomicSwap<(load store seq_cst 4 on %ir.x)> 0x55d93f940c48, FrameIndex:i64<0>, Constant:i32<1065353216>, atomics.zig:157:9
  0x55d93f7602a0: i64 = FrameIndex<0>
  0x55d93f940cb0: i32 = Constant<1065353216>
In function: behavior.atomics.testAtomicRmwFloat

And don't forget that #508 can probably be closed too (and its workaround reverted) 🎉

no-frame-pointer-elim and no-frame-pointer-elim-non-leaf have been
deprecated for a while in favour of the newer (and clearer)
frame-pointer attribute.

Starting with LLVM10 the old attributes are silently ignored, leading to
no stack traces in debug mode.
@andrewrk
Copy link
Member

andrewrk commented Feb 7, 2020

We can't use the baseline_rv64 CPU as-is since lld doesn't support some relocations/relaxations, the trick is to pass an additional -relax to the default feature set. The feature set API is so damn awkward to use that I had to give up after wrestling with it for a while.

So you want to remove "relax" from the "baseline" feature set, right? Here's how to do that:

--- a/lib/std/target/riscv.zig
+++ b/lib/std/target/riscv.zig
@@ -270,7 +270,6 @@ pub const cpu = struct {
             .d,
             .f,
             .m,
-            .relax,
         }),
     };
 
@@ -284,7 +283,6 @@ pub const cpu = struct {
             .d,
             .f,
             .m,
-            .relax,
         }),
     };
 

I was the one who made up that "baseline CPU", so let's just take relax out of it.

@andrewrk andrewrk merged commit a157622 into ziglang:llvm10 Feb 7, 2020
@andrewrk
Copy link
Member

andrewrk commented Feb 7, 2020

And don't forget that #508 can probably be closed too (and its workaround reverted) tada

9e5b248

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Feb 7, 2020

I was the one who made up that "baseline CPU", so let's just take relax out of it.

Oh I thought it was something from LLVM, I'll send a follow-up patch if you want

@andrewrk
Copy link
Member

andrewrk commented Feb 7, 2020

Oh I thought it was something from LLVM, I'll send a follow-up patch if you want

Go for it, because it sounds like you have a follow-up task in mind (enabling some other tests?)

@luismarques
Copy link

Cannot select: 0x55d93f824958: i32,ch = AtomicSwap<(load store seq_cst 4 on %ir.x)> 0x55d93f940c48, FrameIndex:i64<0>, Constant:i32<1065353216>, atomics.zig:157:9
  0x55d93f7602a0: i64 = FrameIndex<0>
  0x55d93f940cb0: i32 = Constant<1065353216>
In function: behavior.atomics.testAtomicRmwFloat

I'll see if I can reproduce this. When I try to build zig against LLVM 10 RC1 it errors out. I'll try to deduce what the LLVM IR was, but if you can provide it it would help.

@luismarques
Copy link

OK, I don't know if it's the same issue but I managed to get an "Unexpected illegal type" assertion with some hand-coded LLVM IR. I'll look into it.

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Feb 9, 2020

target datalayout = "e-m:e-p:64:64-i64:64-i128:128-n64-S128"
target triple = "riscv64-unknown-linux-musl"

define void @_start() #1 {
Entry:
  %x = alloca float, align 4
  store float 0.000000e+00, float* %x, align 4
  %0 = atomicrmw xchg float* %x, float 1.000000e+00 seq_cst
  ret void
}

Compiled using the following cmdline: llc-10 -mattr=+64bit,+a,+c,+d,-e,+f,+m,-relax -O0 -filetype asm -o /tmp/foo.asm /tmp/foo.ll

@luismarques
Copy link

That's the same error I got with the hand-coded .ll file, but it's not a "cannot select" error, as initially reported. I'll look into it, nevertheless. I've never seen floating-point atomics before, I imagine that might be the kind of thing that other frontends deal with by bitcasting to integers first, so this might be off the beaten path.

@LemonBoy
Copy link
Contributor Author

LemonBoy commented Feb 9, 2020

That's the error I got with a non-debug LLVM version, this is the one I get with my other build with debug assertions enabled:

llvm-project/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:971: void {anonymous}::SelectionDAGLegalize::LegalizeOp(llvm::SDNode*): Assertion `TLI.getTypeAction(*DAG.getContext(), Node->getValueType(i)) == TargetLowering::TypeLegal && "Unexpected illegal type!"' failed.

@luismarques
Copy link

Ah, right, I was also using a debug build, and got that error. That explains the difference. Thanks, I'll look into this.

@andrewrk
Copy link
Member

andrewrk commented Feb 17, 2020

I remember why I added "relax" to the baseline feature set. Here's the diff I just tried to do in the llvm10 branch:

diff --git a/lib/std/target/riscv.zig b/lib/std/target/riscv.zig
index efb71a35e..d66cc5742 100644
--- a/lib/std/target/riscv.zig
+++ b/lib/std/target/riscv.zig
@@ -263,7 +263,7 @@ pub const all_features = blk: {
 pub const cpu = struct {
     pub const baseline_rv32 = Cpu{
         .name = "baseline_rv32",
-        .llvm_name = "generic-rv32",
+        .llvm_name = null,
         .features = featureSet(&[_]Feature{
             .a,
             .c,
@@ -275,7 +275,7 @@ pub const cpu = struct {
 
     pub const baseline_rv64 = Cpu{
         .name = "baseline_rv64",
-        .llvm_name = "generic-rv64",
+        .llvm_name = null,
         .features = featureSet(&[_]Feature{
             .@"64bit",
             .a,
@@ -288,14 +288,14 @@ pub const cpu = struct {
 
     pub const generic_rv32 = Cpu{
         .name = "generic_rv32",
-        .llvm_name = "generic-rv32",
+        .llvm_name = null,
         .features = featureSet(&[_]Feature{
             .rvc_hints,
         }),
     };
     pub const generic_rv64 = Cpu{
         .name = "generic_rv64",
-        .llvm_name = "generic-rv64",
+        .llvm_name = null,
         .features = featureSet(&[_]Feature{
             .@"64bit",
             .rvc_hints,
diff --git a/test/tests.zig b/test/tests.zig
index f079b4664..ad84e2c44 100644
--- a/test/tests.zig
+++ b/test/tests.zig
@@ -196,6 +196,41 @@ const test_targets = blk: {
             .link_libc = true,
         },
 
+        TestTarget{
+            .target = Target{
+                .Cross = CrossTarget{
+                    .os = .linux,
+                    .arch = .riscv64,
+                    .cpu_features = Target.Arch.riscv64.getBaselineCpuFeatures(),
+                    .abi = .none,
+                },
+            },
+        },
+
+        TestTarget{
+            .target = Target{
+                .Cross = CrossTarget{
+                    .os = .linux,
+                    .arch = .riscv64,
+                    .cpu_features = Target.Arch.riscv64.getBaselineCpuFeatures(),
+                    .abi = .musl,
+                },
+            },
+            .link_libc = true,
+        },
+
+        TestTarget{
+            .target = Target{
+                .Cross = CrossTarget{
+                    .os = .linux,
+                    .arch = .riscv64,
+                    .cpu_features = Target.Arch.riscv64.getBaselineCpuFeatures(),
+                    .abi = .gnu,
+                },
+            },
+            .link_libc = true,
+        },
+
         TestTarget{
             .target = Target{
                 .Cross = CrossTarget{

Running it with this command:

./zig build test -Dskip-release -Denable-wine  -Denable-qemu -Denable-foreign-glibc=/home/andy/Downloads/glibc/multi/install/glibcs

Gives this output:

Build Dependencies...c [4] Link...lld: error: /home/andy/tmp/zig/zig-cache/o/IpDYEfWhYXptyo1wUMBOHGigvD1gIwswwGsn9q8PMx_nZWbodvgUyWK82s8FMuVF/test.o: cannot link object files with different floating-point ABI
lld: error: /home/andy/.cache/zig/stage1/o/VEx_936BTIqzzeoCwN8Y-mf4ID7zKTOTVnjO97Lck-YxbGVgANh3e-2BvnjNTl9I/libcompiler_rt.a(/home/andy/.cache/zig/stage1/o/VEx_936BTIqzzeoCwN8Y-mf4ID7zKTOTVnjO97Lck-YxbGVgANh3e-2BvnjNTl9I/compiler_rt.o): cannot link object files with different floating-point ABI
lld: error: crt1.c:(.text+0x0): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/.cache/zig/stage1/o/0DQElJcX1SKnFVMT4eVjKHnwIlJxEkzUcZKZCC4IvAzHfLoW1Qhmft4sIBdlpOCG/libc.a(/home/andy/.cache/zig/stage1/o/RQJU2zWpMxCLAgnzDiigjm77gHOJ4PSso5xecaIEan4FujS6rKR4ofNvnez377ZH/__set_thread_area.o):(.text+0x0): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/.cache/zig/stage1/o/0DQElJcX1SKnFVMT4eVjKHnwIlJxEkzUcZKZCC4IvAzHfLoW1Qhmft4sIBdlpOCG/libc.a(/home/andy/.cache/zig/stage1/o/RQJU2zWpMxCLAgnzDiigjm77gHOJ4PSso5xecaIEan4FujS6rKR4ofNvnez377ZH/__set_thread_area.o):(.text+0x2): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/.cache/zig/stage1/o/0DQElJcX1SKnFVMT4eVjKHnwIlJxEkzUcZKZCC4IvAzHfLoW1Qhmft4sIBdlpOCG/libc.a(/home/andy/.cache/zig/stage1/o/y23enU50XsJY1XTmX8wN98Qif0RzUX49lgc9GC54QyTuBHwdrAEEQa5Cjk5g2wTY/restore.o):(.text+0x0): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/.cache/zig/stage1/o/0DQElJcX1SKnFVMT4eVjKHnwIlJxEkzUcZKZCC4IvAzHfLoW1Qhmft4sIBdlpOCG/libc.a(/home/andy/.cache/zig/stage1/o/y23enU50XsJY1XTmX8wN98Qif0RzUX49lgc9GC54QyTuBHwdrAEEQa5Cjk5g2wTY/restore.o):(.text+0x2): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax

The following command exited with error code 1:
/home/andy/tmp/zig/build-llvm10-release/zig test /home/andy/tmp/zig/test/stage1/behavior.zig --library c --test-name-prefix behavior-riscv64-linux-musl-Debug-c-multi  --cache-dir /home/andy/tmp/zig/zig-cache --name test -target riscv64-linux-musl --test-cmd qemu-riscv64 --test-cmd-bin --override-lib-dir /home/andy/tmp/zig/lib 

So it looks like musl depends on linker relaxation features.

This is a bummer, it means even with LLVM 10 we can't cross compile for -target riscv64-linux-musl. Re-running the tests with "relax" added back to the baseline target features, we get:

lld: error: test:(.text+0x0): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11BBC): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11BF2): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11F00): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11F1E): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11F8C): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11FBA): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x11FD2): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax
lld: error: /home/andy/tmp/zig/lib/std/heap.zig:701:(.text+0x121AA): relocation R_RISCV_ALIGN requires unimplemented linker relaxation; recompile with -mno-relax

The following command exited with error code 1:
/home/andy/tmp/zig/build-llvm10-release/zig test /home/andy/tmp/zig/test/stage1/behavior.zig --test-name-prefix behavior-riscv64-linux-none-Debug-bare-multi  --cache-dir /home/andy/tmp/zig/zig-cache --name test -target riscv64-linux-none --test-cmd qemu-riscv64 --test-cmd-bin --override-lib-dir /home/andy/tmp/zig/lib

@luismarques
Copy link

I remember why I added "relax" to the baseline feature set.

I get the impression you're saying you added "relax" to solve this issue but it might be the other way around. Isn't it the case that you compile musl with code relaxation enabled, thus generating R_RISCV_ALIGNs relocations? You then need a linker which supports relaxations to handle that.

Can't you compile musl without generating those align relocations? I don't remember 100% of the top of my head if disabling code relaxations is sufficient to ensure that, but that was my impression. If you don't have the align relocations you can then use LLD, for a pure LLVM toolchain.

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.

3 participants