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.

Sign up for GitHub

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

Jump to bottom

Remove heaps from core Cranelift, push them into cranelift-wasm #5386

Merged
merged 42 commits into from
Dec 15, 2022
Merged

Remove heaps from core Cranelift, push them into cranelift-wasm #5386

merged 42 commits into from
Dec 15, 2022

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Dec 6, 2022 •
edited
Loading

Still need to port filetests that use heaps over to a new thing, which I need to build and will land as a PR before this one.

Fixes #5283

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:docs cranelift:meta Everything related to the meta-language. labels Dec 6, 2022
@fitzgen fitzgen marked this pull request as ready for review December 13, 2022 20:10
@fitzgen fitzgen requested a review from cfallin December 13, 2022 20:10
@fitzgen
Copy link
Member Author

fitzgen commented Dec 13, 2022

@cfallin I think this is ready for review! Sorry it got a bit big, probably best to go through commit by commit.

cfallin
cfallin approved these changes Dec 13, 2022
View reviewed changes
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This is great -- thanks for the careful and methodical work here and especially the attention to the testing! A few thoughts below but no showstoppers. r=me with comments addressed

cranelift/docs/ir.md Show resolved Hide resolved
...rch64/wasm/load_store_dynamic_kind_i32_index_0_guard_no_spectre_i32_access_0x1000_offset.wat Outdated Show resolved Hide resolved
cranelift/filetests/filetests/runtests/fdemote.clif Show resolved Hide resolved
cranelift/wasm/src/code_translator.rs Show resolved Hide resolved
cranelift/wasm/src/code_translator.rs Outdated Show resolved Hide resolved
cranelift/wasm/src/code_translator/bounds_checks.rs Show resolved Hide resolved
cranelift/wasm/src/code_translator/bounds_checks.rs Show resolved Hide resolved
cranelift/wasm/src/environ/spec.rs Show resolved Hide resolved
@fitzgen fitzgen mentioned this pull request Dec 14, 2022
Merged
@fitzgen
cranelift-wasm: translate Wasm loads into lower-level CLIF operations
5266ea9 
Rather than using `heap_{load,store,addr}`.
@fitzgen
cranelift: Remove the heap_{addr,load,store} instructions
4061db1 
These are now legalized in the `cranelift-wasm` frontend.
@fitzgen
cranelift: Remove the ir::Heap entity from CLIF
900fe99
@fitzgen
Port basic memory operation tests to .wat filetests
6c6cebb
@fitzgen
Remove test for verifying CLIF heaps
eb872f7
@fitzgen
Remove heap_addr from replace_branching_instructions_and_cfg_predec…
2ed1d32 
…essors.clif test
@fitzgen
Remove heap_addr from readonly.clif test
b8f0cdb
@fitzgen
Remove heap_addr from table_addr.clif test
62e7773
@fitzgen
Remove heap_addr from the simd-fvpromote_low.clif test
0c917b0
@fitzgen
Remove heap_addr from simd-fvdemote.clif test
9b5d0e9
@fitzgen
Remove heap_addr from the load-op-store.clif test
feab8f4
@fitzgen
Remove the CLIF heap runtest
f6ba041
@fitzgen
Remove heap_addr from the global_value.clif test
5de2471
@fitzgen
Remove heap_addr from fpromote.clif runtests
2e8408e
@fitzgen
Remove runtests that rely on heaps
18925d7 
Heaps are not a thing in CLIF or the interpreter anymore
@fitzgen
Add generated load/store .wat tests
84ad8bd
@fitzgen
Enable memory-related wasm features in .wat tests
463fd93
@fitzgen
Remove CLIF heap from fcmp-mem-bug.clif test
63140e9
@fitzgen
Add a mode for compiling .wat all the way to assembly in filetests
95b968f
@fitzgen
Also generate WAT to assembly tests in make-load-store-tests.sh
422643d
@fitzgen
cargo fmt
aca6520
@fitzgen
Reinstate f{de,pro}mote.clif tests without the heap bits
0bf088a
@fitzgen
Remove undefined doc link
8dd762f
@fitzgen
Remove outdated SVG and dot file from docs
36ef819
@fitzgen
Add docs about None returns for base address computation helpers
b93ffc2
@fitzgen
Factor out env.heap_access_spectre_mitigation() to a local
d969082
@fitzgen
Expand docs for FuncEnvironment::heaps trait method
5a53cd0
@fitzgen
Restore f{de,pro}mote+load clif runtests with stack memory
1cd66b5
@fitzgen fitzgen enabled auto-merge (squash) December 14, 2022 22:28
@fitzgen fitzgen merged commit c0b587a into bytecodealliance:main Dec 15, 2022
@fitzgen fitzgen deleted the no-heaps-in-clif branch December 15, 2022 03:42
This was referenced Dec 15, 2022
Closed
Closed
@afonso360 afonso360 mentioned this pull request Jan 8, 2023
Merged
@jameysharp jameysharp mentioned this pull request Mar 7, 2023
Closed
@jameysharp jameysharp mentioned this pull request Mar 29, 2023
Open
@afonso360 afonso360 mentioned this pull request Jun 28, 2023
Closed
afonso360 added a commit to afonso360/wasmtime that referenced this pull request Jun 28, 2023
@afonso360
cranelift: Support calling functions with vmctx arguments
570c7b5 
This is a leftover from bytecodealliance#3302 where we used to inject a special `vmctx`
struct if the test requested it. We've removed that capability in bytecodealliance#5386
but accidentally left this in, which caused some weird handling of these
test invocations.
@afonso360 afonso360 mentioned this pull request Jun 28, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jun 28, 2023
@afonso360
cranelift: Support calling functions with vmctx arguments (#6663)
0851e90 
This is a leftover from #3302 where we used to inject a special `vmctx`
struct if the test requested it. We've removed that capability in #5386
but accidentally left this in, which caused some weird handling of these
test invocations.
@afonso360 afonso360 mentioned this pull request Jul 1, 2023
Merged
@afonso360 afonso360 mentioned this pull request Oct 11, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameysharp jameysharp jameysharp left review comments

@afonso360 afonso360 afonso360 left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@cfallin cfallin cfallin approved these changes

Assignees
No one assigned
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:docs cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cranelift: reconsider the heap_addr instruction
5 participants
@fitzgen @cfallin @jameysharp @afonso360 @bjorn3