fitzgen opened PR #5386 from no-heaps-in-clif
to main
:
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.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen edited PR #5386 from no-heaps-in-clif
to main
:
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 #5823
fitzgen edited PR #5386 from no-heaps-in-clif
to main
:
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
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen has marked PR #5386 as ready for review.
fitzgen requested cfallin for a review on PR #5386.
afonso360 submitted PR review.
afonso360 created PR review comment:
Was this file (and
fpromote
) removed by accident?
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Ah good catch, I didn't realize there were non-heap bits in here too.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Can we delete
heap.svg
(andheap.dot
, which I think it was derived from) as well?
cfallin created PR review comment:
Can we document here what a
None
return value means? (I also looked inbounds_checks
andprepare_addr
doesn't seem to say either.) Does it mean that the access always traps (as one might infer from the "set unreachable and return" above)? Or something else?
cfallin created PR review comment:
minor nit: I think the
spectre
local makes the cases a bit more readable below, vs. expanding outenv.enable_heap_access_spectre_mitigation()
everywhere, but I don't feel too strongly about it if you want to inline it...
cfallin created PR review comment:
It's great that we have all of these tests now. One thought: could you say a bit more about what level of scrutiny all of the golden outputs have had? E.g. did you spot-check, or systematically check one variant for each axis, or read every golden output in this diff (props if so!), or...? To be clear I'm totally fine with checking one variant on each axis, given the size of the space we're spanning, I'm just curious for the review-record here.
cfallin created PR review comment:
The lack of heaps for interpreter tests is somewhat unfortunate; though we still do have stackslots (and tables and global values) in the memory scheme that the interpreter uses. I wonder if this test (and fpromote below) could be rewritten to use a stackslot and
stack_addr
, rather than deleted entirely, since the important bit seems to be a test of how certain ops combine with loads/stores?
cfallin created PR review comment:
Can we comment why we determine that the fallthrough point is unreachable here? (This goes along with the request to document what
None
means below.)
cfallin created PR review comment:
A bit more documentation might be good here; otherwise this API is a bit perplexing at first glance (below there is
make_heap
, so the translator is telling the environment about heaps, but here we provide information about heaps back to the translator, which is kind of subtle).Something like: "The returned map should provide heap format details (encoded in
HeapData
) for eachHeap
that was previously returned bymake_heap()
. The translator will first callmake_heap
for each Wasm memory, and then later when translating code, will invokeheaps()
to learn how to access the environment's implementation of each memory."
cfallin created PR review comment:
Also, I'm going back and forth on whether I like having so many individual files or larger consolidated files. On the one hand, I understand why you did it this way (easier to write the shell script to generate them, and it's nice to have separate test cases in separate files for running them individually, and it's easier to write the parser to get the settings once from the top of the file). On the other hand, it might be more approachable overall to have a single large file of autogenerated test cases, rather than a massive directory with intimidating filenames. Thoughts?
cfallin created PR review comment:
Can we document what a
None
return value means?
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen created PR review comment:
Done.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I vastly prefer many small files to one large file. Just so much easier to reason about and debug.
I tried to hide them in their own dedicated directories to at least isolate the "ahhhh!!! so many files!!!!" reaction.
fitzgen created PR review comment:
One thought: could you say a bit more about what level of scrutiny all of the golden outputs have had?
I've spot checked a few of them, mostly Wasm-to-CLIF outputs, definitely haven't gone through all of them.
I think the primary value is for the future: they really shouldn't change unless we expect them to, and when we do improve these sequences we can give the ones that changed a deeper level of scrutiny.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah, it means the access always traps. Will add comments.
jameysharp submitted PR review.
jameysharp created PR review comment:
A suggestion: It might help to land as many of the wasm tests as possible separately, before this PR removes Cranelift's heap support, so we can see whether this PR changes the generated sequences. I think we can't merge any test changes involving the existing
heap_addr
CLIF instruction.But filetests which go from .wat to native assembly should provide good information about the effect of this PR. That way the golden outputs initially are "whatever we do today", and their value as regression tests begins immediately.
Can filetests invoke specific legalization passes? The new wasm-to-CLIF filetests in this PR could also be added beforehand if we can test what the generated CLIF looks like after legalizing heap instructions.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I don't think it can invoke specific legalization passes, but it can test the output of legalization as a whole if you input clif ir rather than wasm.
fitzgen submitted PR review.
fitzgen created PR review comment:
I've pulled out the tests to https://github.com/bytecodealliance/wasmtime/pull/5439 so we can check for regressions and perturbations in this PR after I rebase.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen has enabled auto merge for PR #5386.
fitzgen updated PR #5386 from no-heaps-in-clif
to main
.
fitzgen merged PR #5386.
Last updated: Dec 23 2024 at 12:05 UTC