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.
-->
jlb6740 updated PR #3209 from fix-3161
to main
.
jlb6740 requested abrown for a review on PR #3209.
jlb6740 requested alexcrichton for a review on PR #3209.
cfallin submitted PR review.
cfallin submitted PR review.
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
I16X8
toF64X2
, vs.I32X4
toF64X2
, orI16X8
toF32X4
, or any other combination?
jlb6740 submitted PR review.
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?
cfallin submitted PR review.
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_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 edited PR review comment.
jlb6740 submitted PR review.
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?
cfallin submitted PR review.
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.
alexcrichton submitted PR review.
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::F32X4
from 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?
cfallin submitted PR review.
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.
jlb6740 submitted PR review.
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?
alexcrichton submitted PR review.
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.
jlb6740 updated PR #3209 from fix-3161
to main
.
jlb6740 submitted PR review.
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.
jlb6740 requested cfallin for a review on PR #3209.
jlb6740 updated PR #3209 from fix-3161
to main
.
abrown submitted PR review.
abrown created PR review comment:
I think we need to remove the extract lane part above?
jlb6740 updated PR #3209 from fix-3161
to main
.
jlb6740 created PR review comment:
Yes, I also added more tests. Thanks.
jlb6740 submitted PR review.
jlb6740 requested abrown for a review on PR #3209.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, that's a nice feature!
cfallin merged PR #3209.
Last updated: Dec 23 2024 at 12:05 UTC