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

Apply RLS to @splat builtin, eliminating its length parameter #16346

Merged
merged 6 commits into from
Jul 13, 2023

Conversation

antlilja
Copy link
Contributor

@antlilja antlilja commented Jul 7, 2023

Closes #16277

This PR removes the length parameter from the splat builtin instead resolving the result type.

src/AstGen.zig Outdated Show resolved Hide resolved
@mlugg
Copy link
Member

mlugg commented Jul 7, 2023

Sema logic is missing type checks. Need to check dest ty is a vector

@mlugg
Copy link
Member

mlugg commented Jul 7, 2023

Also, CI failures are because zig1 doesn't have the changes. Run zig build update-zig1 and commit the changes in the stage1 dir (Andrew will recreate this commit before merging for security reasons)

@antlilja antlilja force-pushed the splat-rls branch 3 times, most recently from 6365adb to 29c0910 Compare July 7, 2023 20:37
@andrewrk
Copy link
Member

andrewrk commented Jul 8, 2023

Thanks for implementing this.

Hmmmm, looking at the diff, is this change actually good? It seems kind of worse actually. Are there any more opportunities to clean up the diff so that we can see if this change actually results in more maintainable code?

@mlugg
Copy link
Member

mlugg commented Jul 8, 2023

I think a key thing that would make this nicer to work with is if we can figure out RLS for arithmetic operands, but I'm still unsure what exactly such a proposal would look like

@antlilja antlilja force-pushed the splat-rls branch 2 times, most recently from 64aad35 to 7847120 Compare July 8, 2023 23:22
@antlilja
Copy link
Contributor Author

antlilja commented Jul 8, 2023

Thanks for implementing this.

Hmmmm, looking at the diff, is this change actually good? It seems kind of worse actually. Are there any more opportunities to clean up the diff so that we can see if this change actually results in more maintainable code?

I've now pushed some new changes where I cleaned up the code as much as possible.

I do agree that some parts do look a bit less readable. This is the resulting change when the result type cannot be inferred:

// Before:
@splat(N, @as(T, SOME_LITERAL));

// After:
@as(@Vector(N, T), @splat(SOME_LITERAL));

In quite a few places the code is less readable because the type cannot be inferred where it probably could be. I think @mlugg is correct in that RLS for arithmetic operands would help but there are a lot of places where this wouldn't help. For example where this syntax change interacts with the new cast syntax:

// Before:
@splat(len, @as(i32, @intCast(some_var)))

// After:
@as(@Vector(len, i32), @splat(@as(i32, @intCast(some_var))))

@mlugg
Copy link
Member

mlugg commented Jul 9, 2023

That case can be fixed by adding a result type for @splat's operand using a elem_type_index instruction (or, similarly, by extending elem_type to work with vectors).

@andrewrk
Copy link
Member

Thanks for further exploring it with cleanups, @antlilja. Upon reexamination, I think that this change is at worst, neutral. Given that it increases consistency with other builtins, and sometimes can reduce an awkward need to pass the element type rather than the vector type, I'm in favor of moving forward with it.

@antlilja
Copy link
Contributor Author

Thanks for further exploring it with cleanups, @antlilja. Upon reexamination, I think that this change is at worst, neutral. Given that it increases consistency with other builtins, and sometimes can reduce an awkward need to pass the element type rather than the vector type, I'm in favor of moving forward with it.

Then I'll keep working on it. I've started implementing what mlugg suggested so it works better with the other changes to builtins.

antlilja and others added 6 commits July 12, 2023 15:35
Resolve the result type of the splat builtin instead of requiring a
length parameter.
Needed due to the breaking changes to `@splat` which are used by the
self-hosted compiler.

This update also includes the improvement that allows casting builtins
to infer the result type through optionals and error unions.
@andrewrk andrewrk enabled auto-merge July 12, 2023 22:53
@andrewrk andrewrk merged commit e05c242 into ziglang:master Jul 13, 2023
10 checks passed
@antlilja antlilja deleted the splat-rls branch July 13, 2023 14:21
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.

Proposal: apply RLS to @splat, eliminating its length parameter
3 participants