Optimize lowering of s[start..][0..len]

[dweiller]

Closes #15482.

This PR implements a new ZIR instruction slice_length to facilitate optimizing lowering of the slice-by-length pattern s[start..][0..len].

This PR makes one change to the change to the language (discussion here): you can now slice a multi-pointer using the slice-by-length syntax, i.e. ptr[start..][0..len]; previously, use of this pattern would cause a compile error due to slicing multi-pointers without a end being an error.

Taking the example function in the linked issue:

fn foo(s: []const i32, start: usize, len: usize) []const i32 {
    return s[start..][0..len];
}

The core change this makes in the ZIR is:

- %16 = dbg_stmt(2, 13)
- %17 = slice_start(%15, %6) node_offset:2:12 to :2:22
- %18 = ref(%17) token_offset:2:12 to :2:13
- %19 = dbg_stmt(2, 22)
- %20 = slice_end(%18, @Zir.Inst.Refzero, %8) node_offset:2:12 to :2:30
- %21 = as_node(%14, %20) node_offset:2:12 to :2:30
+ %16 = dbg_stmt(2, 22)
+ %17 = slice_length(%15, %6, %8) node_offset:2:12 to :2:30
+ %18 = as_node(%14, %17) node_offset:2:12 to :2:30

with the rest of the diff being instruction renumbering due to the reduced number of ZIR instructions.

The core part of the change in AIR (with -OReleaseFast) is:

- %13!= dbg_stmt(2:13)
  %15 = load([]const 832, %12!)
- %17 = slice_ptr([*]const i32, %15)
+ %17 = slice_ptr([*]const i32, %15!)
- %19 = ptr_add([*]const i32, %17!, %2)
+ %19 = ptr_add([*]const i32, %17!, %2!)
- %20 = slice_len(usize, %15!)
- %21 = sub(%20!, %2!)
- %23 = slice([]const i32, %19!, %21!)
- %24 = alloc(*[]const i32)
- %26!= store(%24, %23!)
- %28 = bitcast(*const []const i32, %24!)
- %29!= dbg_stmt(2:22)
- %31 = load([]const i32, %28!)
- %33 = slice_ptr([*]const i32, %31!)
- %36 = ptr_add([*]const i32, %33!, %34)
- %38 = slice([]const i32, %36!, %3!)
+ %20!= add(%2, %3)
+ %22 = slice([]const i32, %19!, %3!)

Todos before merging:

• figure out if the start_src_node_offset field of the Zir.Inst.SliceLength is needed or can be removed. Result: it seems like this field, along with the .node_offset_slice_start LazySrcLoc variant is the only natural way to track the location, unless someone has a good suggestion or strong dislike of it I'll leave it. As far as I can tell other solutions would require knowing how ZIR instructions are ordered and would likely break with future changes to ZIR.
• support slicing with sentinels
  • s[start..][0..len:sentinel]
  • s[start..:sentinel][0..len]
  • s[start..:sentinal_1][0..len:sentinel_2]

[andrewrk]

Those diffs look promising!

[dweiller]

The implementation for slicing with sentinels is done without adding a new ZIR instruction (there are 255 with the new slice_length one) by adding an optional sentinel ref, but seems to require an extra add instruction in the AIR:

- %21 = slice([]const i32, %19!, %3!)
+ %20!= add(%2, %3)
+ %22 = slice([]const i32, %19!, %3!)

It's possible that this should have always been included for various checks done in analyzeSlice() that I was skipping earlier. I've edited the top post to include this in the main AIR diff there.

[dweiller]

@kristoff-it I couldn't find where exactly the end field of the DocData.Expr.slice union variant is used, so I have probably done the wrong thing when I put the length index in there. The only thing I've found that looked like it was in the exprName() function in lib/docs/main.js, which might mean we need a new union variant?

Edit: After reading through the autodoc source a little more carefully I added a new sliceLength variant to the DocData.Expr union.

[dweiller]

Assuming the CI passes I think this is now ready for review and merging.

In addition to implementing the slice_length ZIR instruction I've also updated the language reference with a short piece about using slice-by-length, and converted all usages I could find (other than those in test/safety) of s[start..start+len] to a slice-by-length.

The added behavior tests do not represent changed behavior but some things that I broke at one point or another - only one or two of these were caught by tests in lib/std and the others weren't covered by any tests.

[andrewrk]

CI failed with a bunch of safety checks like this:

run test std-riscv64-linux-none-Debug: error: thread 687576 panic: index out of bounds: index 10, len 4
/home/ci/actions-runner2/_work/zig/zig/lib/std/net.zig:1704:40: 0x111dc7b in dnsParse__anon_126203 (test)
        try callback(ctx, p[1], p[10..][0..len], r);
                                       ^

[dweiller]

It's odd that half of them are passing - as far as I can tell that's a stdlib test that runs on the other targets as well which is failing. I also can't seem to reproduce it locally.

[dweiller]

CI failed with a bunch of safety checks like this:

run test std-riscv64-linux-none-Debug: error: thread 687576 panic: index out of bounds: index 10, len 4
/home/ci/actions-runner2/_work/zig/zig/lib/std/net.zig:1704:40: 0x111dc7b in dnsParse__anon_126203 (test)
        try callback(ctx, p[1], p[10..][0..len], r);
                                       ^

Hopefully I found it - I don't think I had covered multi-pointers properly, which leads to a question: do we want to allow:

fn foo(ptr: [*]const i32, start: usize, len: usize) []const i32 {
    return ptr[start..][0..len];
}

I don't see a reason not to but it is a change to what the language permits - previously this wouldn't be possible as the first slice would error due to multi-pointers not having a length.

[andrewrk]

Ah, that's a great question. Yes, I think this should be allowed.

Arguably, even ptr[start..] should be allowed, which should do the same thing as ptr + start. But that can be a separate enhancement to this patch.

[dweiller]

Ah, that's a great question. Yes, I think this should be allowed.

Arguably, even ptr[start..] should be allowed, which should do the same thing as ptr + start. But that can be a separate enhancement to this patch.

I've updated the PR to allow this, along with some behavior tests for it.

There is discussion related to this in #14611, in fact this PR (with allowing multi-pointer slicing as p[start..][0..len]) covers the use case in the example in that issue so merging could trigger accepting/closing that issue.

[andrewrk]

Looks like the new behavior test coverage didn't pass for our self-hosted WebAssembly backend. You are welcome to disable the entire behavior test for that backend and then a contributor to that backend (perhaps @Luukdegram?) can look at it at their leisure.

By the way, you can find out about failures like this without waiting for the CI by running the behavior tests locally, like this:

zig build test-behavior -fwasmtime -fqemu

You'll need wasmtime and qemu installed when using those respective flags.

[Luukdegram]

Looks like the new behavior test coverage didn't pass for our self-hosted WebAssembly backend. You are welcome to disable the entire behavior test for that backend and then a contributor to that backend (perhaps @Luukdegram?) can look at it at their leisure.

Will be creating a fix along with the work I'm doing to enable the full test runner for the Webassembly backend as it depends on code that uses this pattern.

[dweiller]

I'm confident that the CI failures are issues with the CI and not the PR - both failing jobs hit the
/home/ci/actions-runner1/_work/zig/zig/test/behavior/defer.zig:120:40: error: expected type 'error{One}', found 'void'
error that a number of jobs have been mysteriously failing with - in particular the aarch64-linux-release job passed last time.

[andrewrk]

Nice work, I'll take a look into merging this today.

Good news - @jacobly0 fixed those CI false positives over in #15591.

[dweiller]

Rebased to check the CI fully passes after #15591.

[andrewrk]

Great work!