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

make @boolToInt always return a u1 #15701

Merged
merged 14 commits into from
May 24, 2023
Merged

Conversation

tisonkun
Copy link
Contributor

This closes #7950.

cc @andrewrk @mlugg in case this patch helps.

Copy link
Member

@Vexu Vexu left a comment

Choose a reason for hiding this comment

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

Small style nits.

lib/std/math/ilogb.zig Outdated Show resolved Hide resolved
src/Sema.zig Outdated Show resolved Hide resolved
src/Sema.zig Outdated Show resolved Hide resolved
@Vexu Vexu force-pushed the bool-to-int-u1 branch from bcaaf88 to a13f040 Compare May 14, 2023 09:46
@tisonkun
Copy link
Contributor Author

There seems some more type inconsistent. I'm handling it:

zig-local-cache/o/544ff04ac1c968334ce79488ef10cbe8/source.zig:1137:40: error: expected signed integer type, found 'u1'
zig-local-cache/o/13471bdccffe1aa306a0cac07212bc34/source.zig:1128:39: error: expected signed integer type, found 'u1'
zig-local-cache/o/13471bdccffe1aa306a0cac07212bc34/source.zig:1131:49: error: expected signed integer type, found 'u1'
/Users/runner/work/zig/zig/lib/std/math.zig:1687:57: error: overflow of integer type 'u1' with value '-1'
/Users/runner/work/zig/zig/lib/std/math.zig:1770:41: note: called from here

@Vexu could you avoid force push before we are pending for merging? Otherwise it's hard for me to fetch your changes.

@tisonkun
Copy link
Contributor Author

/Users/m1/actions-runner/_work/zig/zig/lib/std/math.zig:1687:46: error: type 'i1' cannot represent integer value '1'

My bad, lol.

@tisonkun
Copy link
Contributor Author

docs success
+- run docgen (langref.html) success 4m MaxRSS:201M
   +- zig build-exe docgen Debug native success 7s MaxRSS:144M
zig-local-cache/o/2123484352fb62c7b0823ad30f8e43aa/source.zig:755:40: error: expected signed integer type, found 'u1'
zig-local-cache/o/50f4fe28c54b07b279066653c1166ad6/source.zig:746:39: error: expected signed integer type, found 'u1'
zig-local-cache/o/50f4fe28c54b07b279066653c1166ad6/source.zig:749:49: error: expected signed integer type, found 'u1'
Error: Process completed with exit code 2.

A new issue without source code location. Let me try to reproduce it locally.

@Vexu
Copy link
Member

Vexu commented May 15, 2023

There are plenty of other error you might want to start with if you skip the 3000 lines of noise.

Signed-off-by: tison <[email protected]>
@tisonkun
Copy link
Contributor Author

Fix the test result issue. For translate C issue, locally it logs:

/Users/chenzili/.cache/zig/o/6d01653752a9036d704a3412c0963c2b/source.zig:755:40: error: expected signed integer type, found 'u1'
    if (foo(@bitCast(u8, @truncate(i8, @boolToInt(@as(c_int, 1) == @as(c_int, 2))))) != 0) {
                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
    comptime_0: /Users/chenzili/Brittani/zig/build/../lib/std/start.zig:63:50
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

/Users/chenzili/.cache/zig/o/5d933b79a7f095bdc711fe11f6000956/source.zig:746:39: error: expected signed integer type, found 'u1'
    return @bitCast(u8, @truncate(i8, @boolToInt(false or true)));
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~
referenced by:
    main: /Users/chenzili/.cache/zig/o/5d933b79a7f095bdc711fe11f6000956/source.zig:777:11
    comptime_0: /Users/chenzili/Brittani/zig/build/../lib/std/start.zig:63:50
    remaining reference traces hidden; use '-freference-trace' to see all reference traces

/Users/chenzili/.cache/zig/o/5d933b79a7f095bdc711fe11f6000956/source.zig:749:49: error: expected signed integer type, found 'u1'
    return @bitCast(c_short, @truncate(c_short, @boolToInt(@as(c_int, 0) < @as(c_int, 1))));
                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

May I ask where this translate logic stay?

@Vexu
Copy link
Member

Vexu commented May 15, 2023

That is generated with src/translate_c.zig, it's a bit more complex so I can handle it for you if needed.

@tisonkun
Copy link
Contributor Author

tisonkun commented May 15, 2023

@Vexu Thanks for your information! I'll appreciate it if you can help in updating the related logics. I took a look at translate_c.zig and it seems some debug tricks are required to understanding how @bitCast(u8, @truncate(i8 generated. Manually, replace it with @as(u8, ... should work.

@tisonkun
Copy link
Contributor Author

It seems now all aarch64 tests passed but x86_64 ones failed. How can our changes be related to arch?

Related logs:

2023-05-15T16:54:25.7209903Z [16/21] Running zig1.wasm to produce /home/ci/actions-runner5/_work/zig/zig/build-release/zig2.c
2023-05-15T16:54:27.1853797Z [17/21] Building C object CMakeFiles/zig2.dir/compiler_rt.c.o
2023-05-15T16:56:00.2799660Z [18/21] Building C object CMakeFiles/zig2.dir/zig2.c.o
2023-05-15T16:56:00.2800788Z zig2.c:346451:6: warning: incompatible pointer types assigning to 'const uint8_t (*)[14]' (aka 'const unsigned char (*)[14]') from 'const uint8_t *' (aka 'const unsigned char *') [-Wincompatible-pointer-types]
2023-05-15T16:56:00.2801468Z  t53 = (anon__42218_51){(uint8_t const *)((uint8_t const *)&objcopy_ElfFile_28true_29_parse__anon_42242__42242), (uintptr_t)14ul}.ptr;
2023-05-15T16:56:00.2802832Z      ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2023-05-15T16:56:00.2803733Z zig2.c:347145:8: warning: incompatible pointer types assigning to 'uint8_t (*)[4]' (aka 'unsigned char (*)[4]') from 'uint8_t *' (aka 'unsigned char *') [-Wincompatible-pointer-types]
2023-05-15T16:56:00.2804229Z   t124 = t45.ptr;
2023-05-15T16:56:00.2804459Z        ^ ~~~~~~~
2023-05-15T16:56:00.2805236Z zig2.c:348351:6: warning: incompatible pointer types assigning to 'const uint8_t (*)[14]' (aka 'const unsigned char (*)[14]') from 'const uint8_t *' (aka 'const unsigned char *') [-Wincompatible-pointer-types]
2023-05-15T16:56:00.2805890Z  t53 = (anon__42328_51){(uint8_t const *)((uint8_t const *)&objcopy_ElfFile_28true_29_parse__anon_42242__42242), (uintptr_t)14ul}.ptr;
2023-05-15T16:56:00.2806313Z      ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2023-05-15T16:56:00.2807313Z zig2.c:349054:8: warning: incompatible pointer types assigning to 'uint8_t (*)[4]' (aka 'unsigned char (*)[4]') from 'uint8_t *' (aka 'unsigned char *') [-Wincompatible-pointer-types]
2023-05-15T16:56:00.2807758Z   t123 = t45.ptr;
2023-05-15T16:56:00.2807946Z        ^ ~~~~~~~
2023-05-15T16:56:00.2808141Z 4 warnings generated.
2023-05-15T16:56:12.5592729Z [19/21] Linking CXX executable zig2
2023-05-15T17:30:05.8667933Z [20/21] Building stage3
2023-05-15T17:30:05.8861337Z [20/21] Install the project...
2023-05-15T17:30:05.9071517Z -- Install configuration: "Release"
2023-05-15T17:30:07.4168309Z + echo Looking for non-conforming code formatting...
2023-05-15T17:30:07.4169112Z Looking for non-conforming code formatting...
2023-05-15T17:30:07.4173688Z + stage3-release/bin/zig fmt --check .. --exclude ../test/cases/ --exclude ../build-debug --exclude ../build-release
2023-05-15T17:30:09.9443785Z + stage3-release/bin/zig build -Dtarget=arm-linux-musleabihf
2023-05-15T17:42:01.4598028Z + pwd
2023-05-15T17:42:01.4602291Z + stage3-release/bin/zig build test docs --maxrss 21000000000 -fqemu -fwasmtime -Dstatic-llvm -Dtarget=native-native-musl --search-prefix /home/ci/deps/zig+llvm+lld+clang-x86_64-linux-musl-0.11.0-dev.1869+df4cfc2ec --zig-lib-dir /home/ci/actions-runner5/_work/zig/zig/build-release/../lib
2023-05-15T17:46:49.2837231Z zig test test-c-abi-arm-linux-musleabihf-ReleaseFast ReleaseFast arm-linux-musleabihf: error: warning(link): unexpected LLD stderr:
2023-05-15T17:46:49.2839561Z ld.lld: warning: Linking two modules of different target triples: '/home/ci/actions-runner5/_work/zig/zig/build-release/zig-local-cache/o/fb7621e59f8626159be78416dc8c85b2/test-c-abi-arm-linux-musleabihf-ReleaseFast.o' is 'arm-unknown-linux-musleabihf' whereas 'ld-temp.o' is 'armv6kz-unknown-linux-musleabihf'
2023-05-15T17:46:49.2983730Z 
2023-05-15T17:46:49.2984194Z 
2023-05-15T17:46:49.2984553Z 
2023-05-15T17:48:28.4640985Z run test behavior-x86_64-linux-none-Debug-selfhosted: error: expected Illegal instruction at address 0x80b28c5
2023-05-15T17:48:28.4642533Z run test behavior-x86_64-linux-none-Debug-selfhosted: error: while executing test 'test.cast bool to int', the following command terminated with signal 4 (expected exited with code 0):
2023-05-15T17:48:28.4643617Z /home/ci/actions-runner5/_work/zig/zig/build-release/zig-local-cache/o/62f4419403852e5de38dd4ceeda3d227/test --listen=- 
2023-05-15T20:38:31.3948363Z Build Summary: 4399/4543 steps succeeded; 141 skipped; 1 failed; 143069/149266 tests passed; 6197 skipped (disable with -fno-summary)
2023-05-15T20:38:31.3948854Z test transitive failure
...
error: the following build command failed with exit code 1:
/home/ci/actions-runner5/_work/zig/zig/build-release/zig-local-cache/o/865459311d79db599ea2170f025eac9f/build /home/ci/actions-runner5/_work/zig/zig/build-release/stage3-release/bin/zig /home/ci/actions-runner5/_work/zig/zig /home/ci/actions-runner5/_work/zig/zig/build-release/zig-local-cache /home/ci/actions-runner5/_work/zig/zig/build-release/zig-global-cache test docs --maxrss 21000000000 -fqemu -fwasmtime -Dstatic-llvm -Dtarget=native-native-musl --search-prefix /home/ci/deps/zig+llvm+lld+clang-x86_64-linux-musl-0.11.0-dev.1869+df4cfc2ec --zig-lib-dir /home/ci/actions-runner5/_work/zig/zig/build-release/../lib

@jacobly0
Copy link
Member

jacobly0 commented May 18, 2023

How can our changes be related to arch?

Note that not all runners run the same tests, and that aarch64-selfhosted tests are currently disabled, but x86_64-selfhosted is expected to pass many of the behavior tests. That means that there is a bug in the x86_64 backend introduced/uncovered by these changes. Since you added to the failing behavior test, it's likely that the x86_64 backend never passed the new checks.

Specifically, try expectEqual(@as(i1, -1), @bitCast(i1, @boolToInt(t))); is failing due to bad support for i1, and then crashing trying to print i1 for the same reason. The second commit of #15727 already fixes the crash during printing and allows the test failure to be reported. Extending my @bitCast fix to the reuse case additionally fixes the test failure:

diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig
index a258f732f..594a1381a 100644
--- a/src/arch/x86_64/CodeGen.zig
+++ b/src/arch/x86_64/CodeGen.zig
@@ -10105,18 +10105,22 @@ fn airBitCast(self: *Self, inst: Air.Inst.Index) !void {
         const dst_rc = regClassForType(dst_ty);
         const src_rc = regClassForType(src_ty);
         const src_mcv = try self.resolveInst(ty_op.operand);
-        if (dst_rc.supersetOf(src_rc) and self.reuseOperand(inst, ty_op.operand, 0, src_mcv))
-            break :result src_mcv;

         const src_lock = if (src_mcv.getReg()) |reg| self.register_manager.lockReg(reg) else null;
         defer if (src_lock) |lock| self.register_manager.unlockReg(lock);

-        const dst_mcv = try self.allocRegOrMem(inst, true);
-        try self.genCopy(
-            if (!dst_mcv.isMemory() or src_mcv.isMemory()) dst_ty else src_ty,
-            dst_mcv,
-            src_mcv,
-        );
+        const dst_mcv = if (dst_rc.supersetOf(src_rc) and
+            self.reuseOperand(inst, ty_op.operand, 0, src_mcv))
+            src_mcv
+        else dst: {
+            const dst_mcv = try self.allocRegOrMem(inst, true);
+            try self.genCopy(
+                if (!dst_mcv.isMemory() or src_mcv.isMemory()) dst_ty else src_ty,
+                dst_mcv,
+                src_mcv,
+            );
+            break :dst dst_mcv;
+        };

         const dst_signedness =
             if (dst_ty.isAbiInt()) dst_ty.intInfo(self.target.*).signedness else .unsigned;

@jacobly0 jacobly0 mentioned this pull request May 19, 2023
@tisonkun
Copy link
Contributor Author

@jacobly0 Thank you! I've merged the latest master branch. Could you approve the CI workflow here?

@wooster0
Copy link
Contributor

The langref should be updated:

       {#header_open|@boolToInt#}
       <pre>{#syntax#}@boolToInt(value: bool) u1{#endsyntax#}</pre>
       <p>
       Converts {#syntax#}true{#endsyntax#} to {#syntax#}@as(u1, 1){#endsyntax#} and {#syntax#}false{#endsyntax#} to
                   {#syntax#}@as(u1, 0){#endsyntax#}.
       </p>
-      <p>
-      If the value is known at compile-time, the return type is {#syntax#}comptime_int{#endsyntax#}
-          instead of {#syntax#}u1{#endsyntax#}.
-      </p>
       {#header_close#}

Anyway, wouldn't it be better if we just remove @boolToInt from the language?

I would be open to replacing boolToInt with bitCast, but I think I like bitCast a lot more than Andrew does. It would simplify the language a bit though.

I think I have not expressed my love of @bitCast enough. It's great

@tisonkun
Copy link
Contributor Author

tisonkun commented May 21, 2023

@r00ster91 I'm updating the doc...

For drop @boolToInt, I check @bitCast(u1, true) work as expected. If it's accepted, then the work becomes clean up.

Since it has the similar effect of #7950 and #7950 has been accepted, I'll proceed this patch and wait for ideas on dropping @boolToInt and replace its usage as @bitCast. Anyway, this patch align the manner of @boolToInt to @bitCast(u1, boolvar)

@tisonkun
Copy link
Contributor Author

Docs updated.

@tisonkun tisonkun requested a review from Vexu May 21, 2023 12:37
@tisonkun
Copy link
Contributor Author

ping @Vexu @jacobly0 @andrewrk can we make progress here?

@Vexu
Copy link
Member

Vexu commented May 22, 2023

I'd like to hear Andrew's opinion on #15794 first.

@tisonkun
Copy link
Contributor Author

I'd like to hear Andrew's opinion on #15794 first.

@Vexu That sounds reasonable.

Although, as #7950 was accepted and #7950 doesn't conflict #15794, this pull request can even be a prerequisite of #15794 since it fixes all recipients of @boolToInt to expect a u1 instead of comptime_int. It won't be regression which we should later "revert".

@Vexu Vexu enabled auto-merge (squash) May 23, 2023 16:32
@Vexu Vexu merged commit bfe02ff into ziglang:master May 24, 2023
@tisonkun tisonkun deleted the bool-to-int-u1 branch May 24, 2023 01:02
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.

make @boolToInt always return a u1 even when the bool is comptime known
4 participants