cranelift: Implement PartialEq on Ieee{32,64}

[afonso360]

👋 Hey,

This fixes #4828 by implementing PartialEq in terms of float values for the Ieee{32,64} types.

Previously PartialEq was auto derived. This means that it was implemented in terms of PartialEq in a u32.

This is not correct for floats because NaN != NaN.

PartialOrd was manually implemented in 6d50099, but it seems like it was an oversight to leave PartialEq out until now.

The test suite depends on the previous behaviour so we adjust it to keep comparing bits instead of floats.

cc: @jameysharp

[afonso360]

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 #2251 (comment) where it appears that some float behaviour may change according to each arch, so this struct is intended to normalize that behaviour across arches.

[jameysharp]

Fantastic work here, Afonso. Every question I had was already answered either by a comment in the code or by your PR comments; you've already opened an issue about the unimplemented aarch64 comparisons; and you've dramatically expanded the test cases for fcmp.

Now I want a comment on Ieee32/Ieee64 pointing at the discussion you found, but I'm too excited to merge this to wait for that. 😁

[jameysharp]

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

[afonso360]

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.

[jameysharp]

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

[afonso360]

Hah! We implemented the same thing! 😄

Down to the same name and everything!

[jameysharp]

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

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.

[alexcrichton]

FYI I think this is the cause of #4857