Cranelift: NaN canonicalization panics on F128 arithmetic operations

[zzjas]

.clif Test Case

test compile
set enable_nan_canonicalization=true
target s390x

function %fadd_f128(i64, f128, f128) {
block0(v0: i64, v1: f128, v2: f128):
    v3 = fadd v1, v2
    store v3, v0
    return
}

Steps to Reproduce

RUST_BACKTRACE=1 cargo run -p cranelift-tools -- test test.clif

Expected Results

No crash

Actual Results

thread 'worker #0' (2560515) panicked at cranelift/codegen/src/nan_canonicalization.rs:121:13:
Could not canonicalize NaN: Unexpected result type found.

Versions and Environment

Cranelift version or commit: e82a7ab

Extra Info

F16 (on RISC-V with zfh) and F128 (on s390x) are valid floating-point types that pass the is_fp_arith() check in nan_canonicalization.rs but fall through to the panic in add_nan_canon_seq():

wasmtime/cranelift/codegen/src/nan_canonicalization.rs

Lines 92 to 123 in e82a7ab

	match val_type {
	types::F32 => {
	let canon_nan = pos.ins().f32const(Ieee32::NAN);
	if has_vector_support {
	vectorized_scalar_select(pos, canon_nan, types::F32X4);
	} else {
	scalar_select(pos, canon_nan);
	}
	}
	types::F64 => {
	let canon_nan = pos.ins().f64const(Ieee64::NAN);
	if has_vector_support {
	vectorized_scalar_select(pos, canon_nan, types::F64X2);
	} else {
	scalar_select(pos, canon_nan);
	}
	}
	types::F32X4 => {
	let canon_nan = pos.ins().f32const(Ieee32::NAN);
	let canon_nan = pos.ins().splat(types::F32X4, canon_nan);
	vector_select(pos, canon_nan);
	}
	types::F64X2 => {
	let canon_nan = pos.ins().f64const(Ieee64::NAN);
	let canon_nan = pos.ins().splat(types::F64X2, canon_nan);
	vector_select(pos, canon_nan);
	}
	_ => {
	// Panic if the type given was not an IEEE floating point type.
	panic!("Could not canonicalize NaN: Unexpected result type found.");
	}
	}

Adding the following arm will fix the F128 for s390x:

        types::F128 => {
            let nan_const = pos.func.dfg.constants.insert(Ieee128::NAN.into());
            let canon_nan = pos.ins().f128const(nan_const);
            scalar_select(pos, canon_nan);
        }

I'm not sure if F16 needs to be fixed because of #7322.

Thanks for looking into this and I'm happy to submit a PR if the fix to F128 looks ok.

[alexcrichton]

I think it'd be reasonable to add support to the nan_canonicalization.rs pass for this yeah, and if you're willing to make a PR that'd be much appreciated!

[zzjas]

Happy to make a PR! Just to confirm, do you think just fixing for F128 is enough or would it better to fix for both F128 and F16? Thanks!

[alexcrichton]

I'd personally leave that call to you as a subjective balance between implementation complexity and how motivated you are. Cranelift is probably going to have somewhat patchy support for f16/f128 for awhile unless a particularly motivated contributor comes by, so the exact state of support to me I think is fine with whatever so long as it's incrementally moving forward