wasmtime / PR #5386 Remove heaps from core Cranelift, pus... · git-wasmtime

Can we document here what a None return value means? (I also looked in bounds_checks and prepare_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?

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

cfallin created PR review comment:

minor nit: I think the spectre local makes the cases a bit more readable below, vs. expanding out env.enable_heap_access_spectre_mitigation() everywhere, but I don't feel too strongly about it if you want to inline it...

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

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.

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

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?

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

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.)

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

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 each Heap that was previously returned by make_heap(). The translator will first call make_heap for each Wasm memory, and then later when translating code, will invoke heaps() to learn how to access the environment's implementation of each memory."

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

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?

Wasmtime GitHub notifications bot (Dec 13 2022 at 23:33):

cfallin created PR review comment:

Can we document what a None return value means?

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:37):

fitzgen updated PR #5386 from no-heaps-in-clif to main.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:37):

fitzgen created PR review comment:

Done.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:40):

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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:45):

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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:46):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:46):

fitzgen created PR review comment:

Yeah, it means the access always traps. Will add comments.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:57):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 00:57):

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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 08:38):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 08:38):

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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 20:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 14 2022 at 20:06):

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.

Wasmtime GitHub notifications bot (Dec 14 2022 at 21:55):

fitzgen updated PR #5386 from no-heaps-in-clif to main.

Wasmtime GitHub notifications bot (Dec 14 2022 at 22:16):

fitzgen updated PR #5386 from no-heaps-in-clif to main.

Wasmtime GitHub notifications bot (Dec 14 2022 at 22:28):

fitzgen updated PR #5386 from no-heaps-in-clif to main.

Wasmtime GitHub notifications bot (Dec 14 2022 at 22:28):

fitzgen has enabled auto merge for PR #5386.

Wasmtime GitHub notifications bot (Dec 14 2022 at 23:52):

fitzgen updated PR #5386 from no-heaps-in-clif to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 00:26):

fitzgen merged PR #5386.

Last updated: Nov 22 2024 at 16:03 UTC