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

support pointer subtraction #20089

Merged
merged 2 commits into from
Jul 15, 2024
Merged

support pointer subtraction #20089

merged 2 commits into from
Jul 15, 2024

Conversation

wooster0
Copy link
Contributor

Fixes #1738

I don't know if this should introduce a dedicated AIR instruction instead of emitting two int_from_ptr and a sub_wrap. This is also missing safety just like the existing pointer arithmetic but maybe that can be done as part of #1918? Otherwise, if those two things aren't a blocking issue I think this is more or less complete. It seems to work at least.

I also put an emphasis on introducing a clear distinction between "pointer-integer arithmetic" and "pointer subtraction" (pointer-pointer arithmetic) because both of those are "pointer arithmetic" so "pointer arithmetic" on its own becomes ambiguous.

@wooster0
Copy link
Contributor Author

A maintainer will have to repush the "update zig1.wasm" commit.

@Vexu
Copy link
Member

Vexu commented May 30, 2024

I don't know if this should introduce a dedicated AIR instruction instead of emitting two int_from_ptr and a sub_wrap

I don't think a dedicated AIR instruction is needed (unless for safety checks in the backend?) but it should do something at comptime since that is mentioned as a reason against just using @intFromPtr in the issue.

@wooster0
Copy link
Contributor Author

It now works at comptime as well if it knows the byte offset.

@Vexu
Copy link
Member

Vexu commented May 30, 2024

I think it needs to check that the pointers point to the same object for the operation to make sense at comptime. Otherwise it'll have to fall back to doing it at runtime.

@wooster0
Copy link
Contributor Author

Good point. Could also add some basic "safety" for comptime at least, checking overflow.

src/Sema.zig Show resolved Hide resolved
@Vexu
Copy link
Member

Vexu commented May 31, 2024

I'm not sure if the problem is Value.ptrField always returning a new field pointer or the base address check being too strict but I'd expect both of these to work:

comptime {
    var a: struct { a: u32, b: u32 } = undefined;
    const p1 = &a.a;
    const p2 = &a.b;
    @compileLog(p2 - p1); // runtime because p1 and p2 have different base addresses
}

comptime {
    var a: [64][64]u8 = undefined;
    const p1 = &a[0][12];
    const p2 = &a[15][3];
    @compileLog(p2 - p1); // comptime because p1 and p2 are offset from the same comptime alloc
}

@wooster0
Copy link
Contributor Author

Passes now

@Vexu
Copy link
Member

Vexu commented May 31, 2024

But it results in a wrong value since Value.ptrField always sets the byte_offset to 0.

pub fn main() void {
    var a: struct { a: u32, b: u32 } = undefined;
    const p1 = &a.a;
    const p2 = &a.b;
    std.debug.print("{}\n", .{@intFromPtr(p2) - @intFromPtr(p1)}); // 4
}

@wooster0
Copy link
Contributor Author

wooster0 commented May 31, 2024

Well that's true of course.
Looking closer that's only the case for struct. packed struct and extern struct work. For those it knows the byte_offset.

I considered using structFieldOffset in analyzeArithmetic so that I can get a byte offset for structs too (@offsetOf works on structs) but that doesn't seem possible because you can't get the struct type from the pointer type (it's going to be *u32 without referencing the struct in any way).

The other option would be to change ptrField to use some result of structFieldOffset for byte_offset instead of 0 but it's using 0 for byte_offset in multiple places so there must be some good reason, like the result of structFieldOffset not always being correct because it's an undefined memory layout.

So instead I would only allow packed struct and extern struct at comptime and struct is going to happen at runtime.

src/Sema.zig Show resolved Hide resolved
@wooster0 wooster0 force-pushed the ptrsub branch 2 times, most recently from 482d013 to 470e9d6 Compare May 31, 2024 17:52
@wooster0
Copy link
Contributor Author

wooster0 commented May 31, 2024

I reverted the hash_map.zig and zig1.wasm changes and kept the logic simple for now. Follow-up issue can be opened.

Vexu
Vexu previously approved these changes May 31, 2024
@Vexu Vexu dismissed their stale review July 15, 2024 12:16

Result should be object-size units

@wooster0 wooster0 requested review from mlugg, diffuty and Vexu July 15, 2024 14:18
@Vexu Vexu enabled auto-merge (squash) July 15, 2024 15:44
@Vexu Vexu merged commit 888708e into ziglang:master Jul 15, 2024
10 checks passed
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.

support pointer subtraction
5 participants