Skip to content

Conversation

@alexrp
Copy link
Member

@alexrp alexrp commented Nov 29, 2024

See #21818 and commit messages.

(#21818 should remain open after this to track aligning lime1 exactly with the spec when we upgrade to LLVM 20.)

@alexrp alexrp force-pushed the wasm-generic-baseline branch 2 times, most recently from 9faab7a to b9ecd95 Compare November 29, 2024 05:54
@alexrp

This comment was marked as resolved.

@alexrp

This comment was marked as resolved.

@alexrp alexrp force-pushed the wasm-generic-baseline branch from b9ecd95 to abb51dd Compare November 30, 2024 09:22
@alexrp
Copy link
Member Author

alexrp commented Nov 30, 2024

Ok, the memory.copy/memory.fill bug has been fixed. This should be ready for review.

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.

Thanks for looking into this.

Related commits:

1c9bb6a
836f9fc

@andrewrk
Copy link
Member

andrewrk commented Jan 21, 2025

What do you think about my idea to introduce a CPU feature called bulk_memory_len0_noop which indicates the wasm runtime always allows length zero bulk memory operations and treats them as no-ops? This would at least be used by zig1.wasm since we control the wasm2c implementation, and beyond that would be a small form of protest against the spec.

@alexrp
Copy link
Member Author

alexrp commented Jan 21, 2025

What do you think about my idea to introduce a CPU feature called bulk_memory_len0 which indicates the wasm runtime always allows length zero bulk memory operations and treats them as no-ops? This would at least be used by zig1.wasm since we control the wasm2c implementation, and beyond that would be a small form of protest against the spec.

Seems reasonable. I'll add that.

…ill.

Apparently the WebAssembly spec requires these instructions to trap if the
computed memory access could be out of bounds, even if the length is zero.
Really a rather bizarre design choice.
As discussed in #21818, generic is a poor baseline model because that model is a
moving target in LLVM. Instead, use mvp, which has no features enabled.
…embly.

See: WebAssembly/tool-conventions#235

This is not *quite* using the same features as the spec'd lime1 model because
LLVM 19 doesn't have the level of feature granularity that we need for that.
This will be fixed once we upgrade to LLVM 20.

Part of #21818.
@alexrp alexrp force-pushed the wasm-generic-baseline branch from b157cf5 to 5e54390 Compare January 22, 2025 02:04
@alexrp
Copy link
Member Author

alexrp commented Jan 22, 2025

Ok, I've implemented a nontrapping_bulk_memory_len0 feature (named thusly to follow nontrapping_fptoint's precedent), and also implemented nontrapping_fptoint support in wasm2c. I believe wasm2c is now fully lime1-ready; only extended_const is disabled for now due to wasm-opt support.

I did a zig1.wasm update followed by a stage3 bootstrap locally as a smoke test.

@alexrp alexrp requested a review from andrewrk January 22, 2025 03:16
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.

Looks great, thanks!

@alexrp alexrp enabled auto-merge January 22, 2025 04:28
@alexrp alexrp disabled auto-merge January 22, 2025 09:40
This will mainly be used when targeting our wasm2c implementation which has no
problem with zero-length bulk memory operations, as a non-standard extension.
This is more verbose, but at least we now get a compile error instead of UB when
a new feature is added to std.Target.wasm.Feature but not to link.Wasm.Feature.
@alexrp alexrp force-pushed the wasm-generic-baseline branch from 848c589 to 68c6a88 Compare January 22, 2025 20:21
@alexrp alexrp enabled auto-merge January 22, 2025 20:23
@alexrp alexrp merged commit d916954 into ziglang:master Jan 23, 2025
10 checks passed
@alexrp alexrp deleted the wasm-generic-baseline branch January 23, 2025 18:09

pub fn fromCpuFeature(feature: std.Target.wasm.Feature) Tag {
return @enumFromInt(@intFromEnum(feature));
return switch (feature) {
Copy link
Member

Choose a reason for hiding this comment

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

Good move, this looks a lot better. Sorry about the trouble, I should have done this from the beginning.

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