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

llvm: handle vectors in packed structs #14004

Merged
merged 8 commits into from
Dec 20, 2022
Merged

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Dec 19, 2022

No description provided.

@Vexu Vexu force-pushed the packed-struct-vector branch from 9eaa2c2 to 6060bf0 Compare December 19, 2022 15:00
Vexu added 2 commits December 19, 2022 17:01
Vectors can represented in all the same values as arrays
so this was never a valid shortcut.
@Vexu Vexu force-pushed the packed-struct-vector branch from 6060bf0 to 0c1d865 Compare December 19, 2022 15:02
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Everything looks good except for

Sema: add error for recursive inline call

Note that you closed #13724 as a duplicate of the issue closed by this commit, however, the expected behavior is error: comptime execution exceeded 1000 branches, and furthermore the expected behavior is that you can set the branch quota higher and have the code compile successfully.

Unfortunately, we will need to rework Sema to use an explicit stack instead of the compiler's stack in order to solve this issue.

Edit: I forgot to add, thank you for helping the Bun folks get updated 👍

@Vexu
Copy link
Member Author

Vexu commented Dec 20, 2022

Sema: add error for recursive inline call

Note that you closed #13724 as a duplicate of the issue closed by this commit,

My bad, your issue is about comptime calls and the other is about plain inline calls. There is also #13803 about generic calls.

however, the expected behavior is error: comptime execution exceeded 1000 branches, and furthermore the expected behavior is that you can set the branch quota higher and have the code compile successfully.

This specific error is (supposed to be) only for inline calls without comptime parameters which should only be able recurse 0, 1 or infinite times in which case a specific compile error seems more appropriate to me.

Unfortunately, we will need to rework Sema to use an explicit stack instead of the compiler's stack in order to solve this issue.

Definitely, but this would help a little.

@Vexu Vexu force-pushed the packed-struct-vector branch from c959dba to 6511afc Compare December 20, 2022 15:01
@Vexu Vexu enabled auto-merge December 20, 2022 15:01
@andrewrk
Copy link
Member

andrewrk commented Dec 20, 2022

here is a new behavior test that should continue passing:

const std = @import("std");
const expect = std.testing.expect;

inline fn foo(x: i32) i32 {
    if (x <= 0) {
        return 0;
    } else {
        return x * 2 + foo(x - 1);
    }
}

const foo_4 = foo(4);

test "recursive inline function" {
    try expect(foo_4 == 20);
}

If this still passes with your changes, then my mistake, this is ready to be merged. I'll check...

...looks like it still passes.

Here's another test to look at:

const std = @import("std");
const expect = std.testing.expect;

inline fn foo(x: i32) i32 {
    if (x <= 0) {
        return 0;
    } else {
        return x * 2 + foo(x - 1);
    }
}

test "recursive inline function" {
    try expect(foo(4) == 20);
}

This one is also intended to pass, because the callsite of foo uses a comptime value for x, which should propagate into the body of foo, making the recursive call also use a comptime value for x. Currently this test causes a stack overflow on both master and this branch, due to x incorrectly being treated as a runtime value.

It looks like the former test here passes with this branch because you removed the one commit that I pointed out, so from my perspective, this branch is ready to be merged 👍

@Vexu
Copy link
Member Author

Vexu commented Dec 20, 2022

If this still passes with your changes, then my mistake, this is ready to be merged. I'll check...

That is not affected because it's a comptime call.

This one is also intended to pass, because the callsite of foo uses a comptime value for x, which should propagate into the body of foo, making the recursive call also use a comptime value for x. Currently this test causes a stack overflow on both master and this branch, due to x incorrectly being treated as a runtime value.

That is in conflict with #13164 being a bug and IMO would be better handled by #7772

@andrewrk
Copy link
Member

That is in conflict with #13164 being a bug and IMO would be better handled by #7772

const std = @import("std");

inline fn setLimits(min: ?u32, max: ?u32) void {
    if (min != null and max != null) {
        std.debug.assert(min.? <= max.?);
    }
}

pub fn main() void {
    var x: u32 = 42;
    setLimits(x, null);
}

The intended behavior here is that max gets treated as comptime null, which means the condition

    if (min != null and max != null) {

would reduce to

    if (min != null and false) {

The actual issue here was fixed by #13360.

The fix from #13219 was incorrect and I am sorry for not catching it earlier.

@Vexu Vexu merged commit e1345fd into ziglang:master Dec 20, 2022
@Vexu Vexu deleted the packed-struct-vector branch May 27, 2023 11:50
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.

2 participants