Wasmtime GitHub notifications bot (Aug 18 2021 at 22:15): Wasmtime GitHub notifications bot (Aug 18 2021 at 22:15):jlb6740 opened PR #3209 from fix-3161 to main:
Assertion was intended for SIMD lowering of F64x2ConvertLowI32x4U
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:15):jlb6740 updated PR #3209 from fix-3161 to main.
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:17):jlb6740 requested abrown for a review on PR #3209.
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:17):jlb6740 requested alexcrichton for a review on PR #3209.
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:40):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:40):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 18 2021 at 22:40):cfallin created PR review comment:
It looks like, without this check, all of the lowering below is fully independent of the input and output types -- is that right? If so, I'm not sure I fully understand how the lowering remains correct: shouldn't it be different if, e.g., we want to do a widen-and-convert from
I16X8toF64X2, vs.I32X4toF64X2, orI16X8toF32X4, or any other combination?
Wasmtime GitHub notifications bot (Aug 18 2021 at 23:38):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Aug 18 2021 at 23:38):jlb6740 created PR review comment:
@cfallin .. Hi thanks. Yes, the lowering and assertion was there because the lowering was for a very specific instruction F64x2ConvertLowI32x4U, that expects input of I32. The input can't be I16X8. So I see, I think the change is needed at the instruction definition? Or better yet, just put this code in a path that targets I32 input and marks anything else as unimplemented? What does the fuzzer use to generate it's valid input?
Wasmtime GitHub notifications bot (Aug 18 2021 at 23:49):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 18 2021 at 23:49):cfallin created PR review comment:
The fuzzer generates arbitrary CLIF; generally, if the CLIF validator allows an instruction with a particular type, then we should support lowering it. (In other words, the failure mode should always be "fails to validate" rather than "assertion triggered".) If we have a lowering that only supports particular types, then we can add those constraints to the instruction, yes -- that would bring things into
correspondence.This is a bit of a wider net than Wasm-based fuzzing -- I definitely recognize that we've left several corners unfilled in several lowerings because "the Wasm translator would never do that". I guess that's sort of the point now -- we're fuzzing at the CLIF level to find these corners and ensure completeness :-)
And, since we're talking about the
fcvt_from_uintinstruction generally, it seems to me that we don't really want to limit it; e.g. the aarch64 implementation here supports all vector types.So my question is: how much work is it to actually build the other cases out? Is there a simple non-optimized lowering we could use for the other types? Or is it as substantial as this whole 50-line sequence for each case?
Wasmtime GitHub notifications bot (Aug 18 2021 at 23:50):cfallin edited PR review comment.
Wasmtime GitHub notifications bot (Aug 19 2021 at 01:34):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 01:34):jlb6740 created PR review comment:
And, since we're talking about the fcvt_from_uint instruction generally, it seems to me that we don't really want to limit it; e.g. the aarch64 implementation here supports all vector types.
So my question is: how much work is it to actually build the other cases out? Is there a simple non-optimized lowering we could use for the other types? Or is it as substantial as this whole 50-line sequence for each case?
@cfallin .. Yes, unfortunately there is no simple lowering to convert I8 or even I16 .. even with AVX-512. It will take finding some accepted sequence. I can look deeper into this, but in general doing thing with packed in particular bytes is never so straight forward. So ... that is a problem then, right? If we constrain the CLIF to only allow I32 because X64 is just handling the lowering it expects for F64x2ConvertLowI32x4U, while the aarch64 implementation these other type cases those other cases would never get tested by the fuzzer. This is the type of thing that I need to document. Any thoughts?
Wasmtime GitHub notifications bot (Aug 19 2021 at 02:02):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 02:02):cfallin created PR review comment:
Hmm, OK -- thank you for looking into this, and sorry that it's a bit more than we expected! I do think that filling this out to support the other type cases is the right thing to do; the alternative is to restrict the instruction's acceptable types, but then that is a weird incongruity in the CLIF instruction set (most other SIMD instructions are fully polymorphic over vector types). And the general direction of "restrict CLIF to only exactly what Wasm needs" feels wrong too, because it's already (much) more general: e.g. it supports 8/16 bit types and booleans, which aren't used in Wasm at all. If this becomes a huge burden, of course, then we can always reconsider how broadly we define the CLIF ops.
Wasmtime GitHub notifications bot (Aug 19 2021 at 03:02):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 03:02):alexcrichton created PR review comment:
To clarify, I don't think that we have anything generating arbitrary clif right now and running it through the backends. https://github.com/bytecodealliance/wasmtime/issues/3161 was a fuzz-bug generated by generating wasm and feeding it through Cranelift, so this is an assertion being tripped from wasm, not just arbitrary CLIF that a compiler could hypothetically generate.
Along those lines I'm not sure if this is the right fix if this only works for the pair of types that were previously listed here? The conversion here that's tripping the assertion is
types::I16X8 => types::F32X4from adding some debug prints, so if this code only handles the 32x4 => 64x2 case then is it incorrect that the 16x8 => 32x4 case is getting here as well?
Wasmtime GitHub notifications bot (Aug 19 2021 at 04:32):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 04:32):cfallin created PR review comment:
Ah, my apologies, for some reason I had the understanding that the fuzzbug was from the CLIF fuzzer, but you're right, it's from a Wasm translation. Everything said above re: covering CLIF semantics generally is still true, but this is even more short-term important if reachable from Wasm.
Wasmtime GitHub notifications bot (Aug 19 2021 at 05:08):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 05:08):jlb6740 created PR review comment:
Ok thanks all. Then sounds like the solution is to create separate paths, failing as unimplemented/todo's in the path for types i8 and i16. I will make this change if agreed?
Wasmtime GitHub notifications bot (Aug 19 2021 at 14:46):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 14:46):alexcrichton created PR review comment:
Personally I agree with @cfallin that the backends should implement all of clif, and clif should be changed if we otherwise don't want some form of instructions to make it to the backend. I would also agree, though, that fixing the wasm input is most important relative to otherwise ensuring all of clif is handled when there are no actual producers of many clif constructs today.
Given that all simd instructions have already been implemented for wasm in clif I'd probably say the fix here would be to ensure that everything "stays on the rails" where possible in that each individual wasm instruction is implemented but it appears combinations of them are hitting different blocks/panics/etc. Ensuring that each instruction hits its own individual instruction would probably be best, and then future improvements where more cases are handled in more places makes sense to fill out for more clif semantics as well as optimizing wasm.
Wasmtime GitHub notifications bot (Aug 24 2021 at 02:28):jlb6740 updated PR #3209 from fix-3161 to main.
Wasmtime GitHub notifications bot (Aug 24 2021 at 05:23):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Aug 24 2021 at 05:23):jlb6740 created PR review comment:
This and the line above can be written together with a let_chain feature:
#![feature(let_chains)]to the crate attributes to enable`but the feature is experimental.
Wasmtime GitHub notifications bot (Aug 24 2021 at 05:26):jlb6740 requested cfallin for a review on PR #3209.
Wasmtime GitHub notifications bot (Aug 24 2021 at 17:50):jlb6740 updated PR #3209 from fix-3161 to main.
Wasmtime GitHub notifications bot (Aug 24 2021 at 19:03):abrown submitted PR review.
Wasmtime GitHub notifications bot (Aug 24 2021 at 19:03):abrown created PR review comment:
I think we need to remove the extract lane part above?
Wasmtime GitHub notifications bot (Aug 25 2021 at 03:26):jlb6740 updated PR #3209 from fix-3161 to main.
Wasmtime GitHub notifications bot (Aug 25 2021 at 03:30):jlb6740 created PR review comment:
Yes, I also added more tests. Thanks.
Wasmtime GitHub notifications bot (Aug 25 2021 at 03:30):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Aug 25 2021 at 14:20):jlb6740 requested abrown for a review on PR #3209.
Wasmtime GitHub notifications bot (Aug 26 2021 at 15:56):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 26 2021 at 15:56):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 26 2021 at 15:56):cfallin created PR review comment:
Ah, that's a nice feature!
Wasmtime GitHub notifications bot (Aug 26 2021 at 15:57):cfallin merged PR #3209.
Last updated: Nov 22 2024 at 16:03 UTC