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

Also remove BulkMemory feature when running LLVMMemCopyFillLowering #7189

Merged
merged 3 commits into from
Jan 4, 2025

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Jan 4, 2025

This is safe because we have already asserted that there are no passive segments. This causes Binaryen to encode the resulting binary without a DataCount section, making it compatible with engines that don't support bulk memory.

This is safe because we have already asserted that there are no passive
segments. This causes Binaryen to encode the resulting binary without
a DataCount section, making it compatible with engines that don't
support bulk memory.
@dschuff
Copy link
Member Author

dschuff commented Jan 4, 2025

@kripken / @tlively we don't currently have tests that check the output features of the module, or of the binary. Any suggestions on how best to do this?

@kripken
Copy link
Member

kripken commented Jan 4, 2025

If you add --emit-target-features then it emits the features section, and we print that in the output, so it can be detected there. e.g.

$ bin/wasm-opt --emit-target-features test/hello_world.wat --enable-simd --print
(module
 (type $i32_i32_=>_i32 (func (param i32 i32) (result i32)))
 (memory $0 256 256)
 (export "add" (func $add))
 (func $add (param $x i32) (param $y i32) (result i32)
  (i32.add
   (local.get $x)
   (local.get $y)
  )
 )
 ;; features section: mutable-globals, simd, sign-ext
)

That shows the default enabled features + simd that was manually enabled.

If that can't work for some reason, another option is test/unit/test_features.py

@dschuff
Copy link
Member Author

dschuff commented Jan 4, 2025

Ah, great! I'll do that.
One small issue is that update_lit_checks.py doesn't support updating this test because it ignores the comment that comes after all the semantic module items. I adjusted the comment to reflect that.

@dschuff dschuff requested a review from kripken January 4, 2025 01:37
@dschuff dschuff enabled auto-merge (squash) January 4, 2025 01:37
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Another option might be a separate small test for the features section, which would allow the main test to still be auto-updateable, but I don't feel strongly.

@dschuff dschuff merged commit 1911e0b into main Jan 4, 2025
13 checks passed
@dschuff dschuff deleted the bulkmem branch January 4, 2025 02:10
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