Wasmtime GitHub notifications bot (Apr 03 2024 at 00:44): Wasmtime GitHub notifications bot (Apr 03 2024 at 00:44):jameysharp requested cfallin for a review on PR #8293.
Wasmtime GitHub notifications bot (Apr 03 2024 at 00:44):jameysharp requested wasmtime-compiler-reviewers for a review on PR #8293.
Wasmtime GitHub notifications bot (Apr 03 2024 at 00:44):jameysharp opened PR #8293 from jameysharp:cleanup-results to bytecodealliance:main:
In particular, remove support for detaching/attaching/appending instruction results.
The AliasAnalysis pass used detach_results, but leaked the detached ValueList; using clear_results instead is better.
The verifier's
test_printing_contextual_errorsneeded to get the verifier to produce an error containing a pretty-printed instruction, and did so by appending too many results. Instead, failing to append any results gets a similar error out of the verifier, without requiring that we expose the easy-to-misuse append_result method. However,iconstis not a suitable instruction for this version of the test because its result type is its controlling type, so failing to create any results caused assertion failures rather than the desired verifier error. I switched tof64constwhich has a non-polymorphic type.The DFG's
aliasestest cleared both results of an instruction and then reattached one of them. Since we have access to DFG internals in these tests, it's easier to directly manipulate the relevant ValueList than to use these unsafe methods.The only other use of attach/append was in
make_inst_results_reusingwhich decided which to use based on whether a particular result was supposed to reuse an existing value. Inlining both methods there revealed that they were nearly identical and could have most of their code factored out.While I was looking at uses of
DataFlowGraph::results, I also simplified replace_with_aliases a little bit.
Wasmtime GitHub notifications bot (Apr 03 2024 at 15:18):cfallin submitted PR review:
Thanks for this cleanup! A few things below but in general I'm always happy when we can shrink an API :-)
Wasmtime GitHub notifications bot (Apr 03 2024 at 15:18):cfallin created PR review comment:
The bound
(dest, original)names are slightly confusing given we're iterating overzip(dest_results, src_results)-- can we pick eitheroriginalorsrcand use it in both places?
Wasmtime GitHub notifications bot (Apr 03 2024 at 15:18):cfallin submitted PR review:
Thanks for this cleanup! A few things below but in general I'm always happy when we can shrink an API :-)
Wasmtime GitHub notifications bot (Apr 03 2024 at 15:18):cfallin created PR review comment:
(I realize this is inlined code, but thoughts still follow:)
The
as u16cast here is indeed protected by the assert above, but it's still something that catches my eye and takes a bit to understand safety thereof -- could we instead do thetry_fromhere (overhead is the same in the common case of one result)?Also, it occurs to me that we want this check even in release builds (if other limits somehow let through a many-result function, we don't want to rely on fuzzing catching a wraparound-caused miscompile before users do); so either a
try_from().expect()or a fullassert!is probably what we want.
Wasmtime GitHub notifications bot (Apr 03 2024 at 17:29):jameysharp updated PR #8293.
Wasmtime GitHub notifications bot (Apr 03 2024 at 17:30):jameysharp commented on PR #8293:
Good suggestions, thanks!
Wasmtime GitHub notifications bot (Apr 03 2024 at 17:30):jameysharp has enabled auto merge for PR #8293.
Wasmtime GitHub notifications bot (Apr 03 2024 at 18:15):jameysharp merged PR #8293.
Last updated: Dec 23 2024 at 12:05 UTC