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

RISC-V behavior tests disabled #3338

Closed
andrewrk opened this issue Sep 29, 2019 · 9 comments
Closed

RISC-V behavior tests disabled #3338

andrewrk opened this issue Sep 29, 2019 · 9 comments

Comments

@andrewrk
Copy link
Member

@andrewrk andrewrk commented Sep 29, 2019

These are all upstream issues with LLVM.

@newStackCall causes:

LLVM ERROR: Named registers not implemented for this target

Issues with f16:

LLVM ERROR: Cannot select: 0x39d32ee0: f32,ch = load<(dereferenceable load 2 from %ir.a), anyext from f16> 0x39d29678, FrameIndex:i64<0>, undef:i64, widening.zig:23:18
  0x39d1aa68: i64 = FrameIndex<0>
  0x38d69710: i64 = undef
In function: behavior.widening.test "float widening"
LLVM ERROR: Cannot select: 0x393500f8: f32 = fp16_to_fp Constant:i64<18988>, widening.zig:40:19
  0x39350298: i64 = Constant<18988>
In function: behavior.widening.test "float widening f16 to f128"
@andrewrk andrewrk added this to the 0.6.0 milestone Sep 29, 2019
andrewrk added a commit that referenced this issue Sep 29, 2019
See #3338 and #3339
@luismarques

This comment has been minimized.

Copy link

@luismarques luismarques commented Sep 30, 2019

Hi. I'm one of the LLVM RISC-V target developers. I tried to diagnose this issue but I can't seem to figure out how to crosscompile the zig test suite. The -target option doesn't seem to work for the test command, and the documentation is scarce.

BTW, I suggest also testing zig releases with debug builds of LLVM. The zig 0.5 release seems to trip a few LLVM asserts, I had to comment-out one of them to get the zig test suite to compile (for the x86 host).

@andrewrk

This comment has been minimized.

Copy link
Member Author

@andrewrk andrewrk commented Sep 30, 2019

Welcome, @luismarques.

The -target option doesn't seem to work for the test command

This functionality has test coverage. What did you try, and what output did you get?

BTW, I suggest also testing zig releases with debug builds of LLVM. The zig 0.5 release seems to trip a few LLVM asserts, I had to comment-out one of them to get the zig test suite to compile (for the x86 host).

I did test this, with the merge of the llvm9 branch. If Zig is tripping any LLVM asserts, that is a regression since e81156b.

documentation is scarce

Have a look at https://github.com/ziglang/zig/blob/master/CONTRIBUTING.md. Ideally we would be able to do this to RISC-V test coverage:

--- a/test/tests.zig
+++ b/test/tests.zig
@@ -70,6 +70,36 @@ const test_targets = [_]TestTarget{
         .link_libc = true,
     },
 
+    TestTarget{
+        .target = Target{
+            .Cross = CrossTarget{
+                .os = .linux,
+                .arch = builtin.Arch.riscv64,
+                .abi = .none,
+            },
+        },
+    },
+    TestTarget{
+        .target = Target{
+            .Cross = CrossTarget{
+                .os = .linux,
+                .arch = builtin.Arch.riscv64,
+                .abi = .musl,
+            },
+        },
+        .link_libc = true,
+    },
+    TestTarget{
+        .target = Target{
+            .Cross = CrossTarget{
+                .os = .linux,
+                .arch = builtin.Arch.riscv64,
+                .abi = .gnu,
+            },
+        },
+        .link_libc = true,
+    },
+
     TestTarget{
         .target = Target{
             .Cross = CrossTarget{

If you apply this patch and then:

./zig build test -Denable-qemu

This will reveal RISC-V problems, when you re-enable the tests that are disabled. You can find these by grepping for riscv (from the build directory):

$ grep -RI riscv ../test/
../test/stage1/behavior/new_stack_call.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/math.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/math.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/math.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/cast.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/widening.zig:    if (@import("builtin").arch == .riscv64) {
../test/stage1/behavior/widening.zig:    if (@import("builtin").arch == .riscv64) {

I'm looking forward to working with you. I'm excited for an open hardware standard such as RISC-V.

@luismarques

This comment has been minimized.

Copy link

@luismarques luismarques commented Sep 30, 2019

This functionality has test coverage. What did you try, and what output did you get?

I tried many combinations, I no longer recall all of them. Here's my reasoning. This works for the host (kind of, it ends up tripping an assertion):

zig-0.5.0/build$ bin/zig build test
(...)
768/768 behavior.widening.test "behavior-x86_64-linux-musl-Debug-c-multi float widening f16 to f128"...OK
All tests passed.
zig: ../lib/CodeGen/GlobalISel/MachineIRBuilder.cpp:907: void llvm::MachineIRBuilder::validateSelectOp(const llvm::LLT &, const llvm::LLT &, const llvm::LLT &, const llvm::LLT &): Assertion `(ResTy == Op0Ty && ResTy == Op1Ty) && "type mismatch"' failed.

...and --help says this:

Compile Options:
(...)
  -target [name]               <arch><sub>-<os>-<abi> see the targets command

Therefore I would expect something like this to work:

$ bin/zig build -target riscv64-linux test
Unrecognized argument: -target
$ bin/zig -target riscv64-linux build test
Unrecognized command: build
$ bin/zig build test -target riscv64-linux
Unrecognized argument: -target

I did test this, with the merge of the llvm9 branch. If Zig is tripping any LLVM asserts, that is a regression since e81156b.

You can download http://releases.llvm.org/9.0.0/llvm-9.0.0.src.tar.xz and http://releases.llvm.org/9.0.0/cfe-9.0.0.src.tar.xz and reproduce this issue. I used the following llvm/cfe cmake configurations:

cmake -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
  -DBUILD_SHARED_LIBS=True -DLLVM_USE_SPLIT_DWARF=True \
  -DLLVM_BUILD_TESTS=True \
  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DLLVM_ENABLE_LLD=True \
  -DLLVM_TARGETS_TO_BUILD="all" \
  -DLLVM_APPEND_VC_REV=False \
  -DLLVM_ENABLE_LIBXML2=OFF \
  -DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="AVR" \
  -DCMAKE_INSTALL_PREFIX=$HOME/tools/llvm-9 -DCMAKE_PREFIX_PATH=$HOME/tools/llvm-9 \
  ..

cmake  -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
  -DBUILD_SHARED_LIBS=True -DLLVM_USE_SPLIT_DWARF=True \
  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DCMAKE_INSTALL_PREFIX=$HOME/tools/llvm-9 -DCMAKE_PREFIX_PATH=$HOME/tools/llvm-9 \
  ..

Have a look at https://github.com/ziglang/zig/blob/master/CONTRIBUTING.md. Ideally we would be able to do this to RISC-V test coverage:

I will continue to take a look at this, as time permits. Thanks.

I'm looking forward to working with you. I'm excited for an open hardware standard such as RISC-V.

Likewise!

@andrewrk

This comment has been minimized.

Copy link
Member Author

@andrewrk andrewrk commented Oct 1, 2019

I fixed the LLVM assertion failure in a8d6954. I'm running the full test suite locally with a debug build of LLVM to find if there were any other regressions. (edit: found one: #3358)

bin/zig build tests all the targets, not just one. Here's the most direct way to reproduce this issue:

  1. Apply the following patch:
diff --git a/test/stage1/behavior/cast.zig b/test/stage1/behavior/cast.zig
index 8176f5cf..04c7fa60 100644
--- a/test/stage1/behavior/cast.zig
+++ b/test/stage1/behavior/cast.zig
@@ -247,10 +247,6 @@ fn testPeerErrorAndArray2(x: u8) anyerror![]const u8 {
 }
 
 test "@floatToInt" {
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
     testFloatToInts();
     comptime testFloatToInts();
 }
diff --git a/test/stage1/behavior/math.zig b/test/stage1/behavior/math.zig
index 1ff63e3e..e285d0eb 100644
--- a/test/stage1/behavior/math.zig
+++ b/test/stage1/behavior/math.zig
@@ -6,10 +6,6 @@ const maxInt = std.math.maxInt;
 const minInt = std.math.minInt;
 
 test "division" {
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
     testDivision();
     comptime testDivision();
 }
@@ -573,10 +569,6 @@ fn remdiv(comptime T: type) void {
 }
 
 test "@sqrt" {
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
     testSqrt(f64, 12.0);
     comptime testSqrt(f64, 12.0);
     testSqrt(f32, 13.0);
@@ -622,10 +614,6 @@ test "vector integer addition" {
 }
 
 test "NaN comparison" {
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
     testNanEqNan(f16);
     testNanEqNan(f32);
     testNanEqNan(f64);
diff --git a/test/stage1/behavior/new_stack_call.zig b/test/stage1/behavior/new_stack_call.zig
index a2f3773a..2fef4816 100644
--- a/test/stage1/behavior/new_stack_call.zig
+++ b/test/stage1/behavior/new_stack_call.zig
@@ -8,11 +8,6 @@ test "calling a function with a new stack" {
     if (@import("builtin").arch == .aarch64) return error.SkipZigTest;
     if (@import("builtin").arch == .mipsel) return error.SkipZigTest;
 
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
-
     const arg = 1234;
 
     const a = @newStackCall(new_stack_bytes[0..512], targetFunction, arg);
diff --git a/test/stage1/behavior/widening.zig b/test/stage1/behavior/widening.zig
index 1aa7ff5d..71f8ae48 100644
--- a/test/stage1/behavior/widening.zig
+++ b/test/stage1/behavior/widening.zig
@@ -19,10 +19,6 @@ test "implicit unsigned integer to signed integer" {
 }
 
 test "float widening" {
-    if (@import("builtin").arch == .riscv64) {
-        // TODO:
-        return error.SkipZigTest;
-    }
     var a: f16 = 12.34;
     var b: f32 = a;
     var c: f64 = b;
@@ -35,10 +31,6 @@ test "float widening" {
 test "float widening f16 to f128" {
     // TODO https://github.com/ziglang/zig/issues/3282
     if (@import("builtin").arch == .aarch64) return error.SkipZigTest;
-    if (@import("builtin").arch == .riscv64) {
-        // TODO: https://github.com/ziglang/zig/issues/3338
-        return error.SkipZigTest;
-    }
 
     var x: f16 = 12.34;
     var y: f128 = x;
  1. Execute the following command (from the build directory):
./zig test ../test/stage1/behavior.zig -target riscv64-linux --test-cmd qemu-riscv64 --test-cmd-bin

The first problem you will run into will be:

LLVM ERROR: Named registers not implemented for this target
@luismarques

This comment has been minimized.

Copy link

@luismarques luismarques commented Oct 2, 2019

I have the zig test suite passing after writing some fixes for these issues. Now I just need to transform the fixes into proper patches, with tests and so on. I'll see if those should be back-ported to LLVM 9.

@andrewrk

This comment has been minimized.

Copy link
Member Author

@andrewrk andrewrk commented Oct 2, 2019

That was fast! Let me know when this stuff makes it into trunk and I'll try a full test suite with building musl & glibc and everything, and try to find if there is anything else.

@luismarques

This comment has been minimized.

Copy link

@luismarques luismarques commented Oct 21, 2019

The patches have been posted for review. When they both land I'll let you know.

@luismarques

This comment has been minimized.

Copy link

@luismarques luismarques commented Nov 28, 2019

I believe this should now work in HEAD. Sorry for not indicating that immediately after the last fix landed.

@andrewrk

This comment has been minimized.

Copy link
Member Author

@andrewrk andrewrk commented Nov 28, 2019

Excellent! I'll leave this issue open until we update llvm dependency to 10.0.0, and enable the tests.

@andrewrk andrewrk mentioned this issue Feb 14, 2020
3 of 13 tasks complete
andrewrk added a commit that referenced this issue Feb 17, 2020
closes #3338
@andrewrk andrewrk closed this in e3447e6 Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.