Stream: git-wasmtime

Topic: wasmtime / issue #4849 cranelift: Implement PartialEq on ...

Wasmtime GitHub notifications bot (Sep 02 2022 at 13:37):

afonso360 commented on issue #4849:

For anyone else wondering why we use Ieee{32,64} like I was (since it looks like we are just slowly reimplementing f32/f64). I found this thread https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646 where it appears that some arches (x86) may silence sNaN's

Wasmtime GitHub notifications bot (Sep 02 2022 at 13:38):

afonso360 edited a comment on issue #4849:

For anyone else wondering why we use Ieee{32,64} like I was (since it looks like we are just slowly reimplementing f32/f64). I found this thread https://github.com/bytecodealliance/wasmtime/pull/2251#discussion_r498508646 where it appears that some float behaviour may change according to each arch, so this struct is intended to normalize that behaviour across arches.

Wasmtime GitHub notifications bot (Sep 02 2022 at 17:58):

jameysharp commented on issue #4849:

Awww, cranelift-fuzzgen needs a fix similar to the test suite runner, because it also expects bitwise equality for NaNs when comparing results.

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:07):

afonso360 commented on issue #4849:

Huh, that is true! I was thinking about implementing a wrapper type over Vec<DataValue> like RunResult or something like that so that we don't have to duplicate the comparison function, what do you think about that?

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:08):

afonso360 edited a comment on issue #4849:

Huh, that is true! I was thinking about implementing a wrapper type over Vec<DataValue> like RunResult or something like that so that we don't have to duplicate the comparison function, what do you think about that?

Edit: Although Result has its own meaning, so we that's probably not the best name. And we already have a RunResult in fuzzgen.

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:20):

jameysharp commented on issue #4849:

I have a fix I'm testing now. I've added a bitwise_eq method to DataValue to share most of the logic. Then in cranelift-fuzzgen I've implemented PartialEq on RunResult in terms of bitwise_eq, and written similar code in compare_results. I don't feel bad about duplicating l.len() == r.len() && l.iter().zip(r).all(|(l, r)| l.bitwise_eq(r)).

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:20):

afonso360 commented on issue #4849:

Hah! We implemented the same thing!


Down to the same name and everything!

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:21):

afonso360 edited a comment on issue #4849:

Hah! We implemented the same thing! :big_smile:


Down to the same name and everything!

Wasmtime GitHub notifications bot (Sep 02 2022 at 18:28):

jameysharp commented on issue #4849:

Hah, nice! Ooh, I like your doc comment better than mine though. :grin:

BTW I only noticed while looking into this that compare_results doesn't check if the vectors have the same length. Maybe there's a check earlier that makes it unnecessary? But anywhere I see .zip, if different lengths could matter then I like to see an explicit check.

Do you want to open a PR for this? Here's the key bit I did in cranelift-fuzzgen:

impl PartialEq for RunResult {
    fn eq(&self, other: &Self) -> bool {
        if let (RunResult::Success(l), RunResult::Success(r)) = (self, other) {
            l.len() == r.len() && l.iter().zip(r).all(|(l, r)| l.bitwise_eq(r))
        } else {
            false
        }
    }
}

Then remove the RunResult::unwrap implementation and just do assert_eq!(int_res, host_res); at the end.

Wasmtime GitHub notifications bot (Sep 02 2022 at 21:03):

alexcrichton commented on issue #4849:

FYI I think this is the cause of https://github.com/bytecodealliance/wasmtime/issues/4857

Last updated: Nov 22 2024 at 16:03 UTC