[codegen] legalize imul for 64-bit and 128-bit operands

[ryzokuken]

Add a legalization that legalizes imul.I64 for 32-bit ISAs.

Refs: bnjbvr/cranelift-x86#4

/cc @wingo @caitp

@bnjbvr PTAL.

There's one small problem though, there's no existing way to handle carry output in imul. Good news? According to https://www.felixcloutier.com/x86/imul, imul does set the appropriate flags on x86-32 atleast, so I can just add an alternate imul_ifcout instruction, I guess? wdyt?

[bjorn3]

Have you seen https://docs.rs/cranelift-codegen/0.44.0/cranelift_codegen/ir/trait.InstBuilder.html#method.umulhi and https://docs.rs/cranelift-codegen/0.44.0/cranelift_codegen/ir/trait.InstBuilder.html#method.smulhi ?

[ryzokuken]

@bjorn3 this is exactly what I needed! Thanks.

[bjorn3]

On x86 you can directly use x86_u/smulx to get both low and hi bits at once:

https://github.com/CraneStation/cranelift/blob/6ed06a1e8e428aedb7adfd825b131abb9a7a3d15/cranelift-codegen/meta/src/isa/x86/legalize.rs#L85

[bjorn3]

Is it just me, or does it need to be more complex. compiler-builtins uses 18 lines to implement this: https://github.com/rust-lang/compiler-builtins/blob/462b73c1fe1f67a62223a3ccf830f02a2571c016/src/int/mul.rs#L7-L26

[bjorn3]

Could you please also implement this and further i64 -> i32 narrowing legalizations for i128?

[ryzokuken]

I could legalize imul.ty in terms of imul.ty_half which is good, but I guess I'll need to legalize umulhi for I64 for that too?

[bjorn3]

For x86_64 it is already implemented: https://github.com/CraneStation/cranelift/blob/6ed06a1e8e428aedb7adfd825b131abb9a7a3d15/cranelift-codegen/meta/src/isa/x86/encodings.rs#L725

32bit support is not needed for me at least for now.

[ryzokuken]

so umulhi is defined for x86_64. I guess it's okay to add the legalization since it's not defined for any other ISA anyway...

[ryzokuken]

@bjorn3 @bnjbvr done, with tests and all.

[bjorn3]

Could you please add a run test with very large 128bit ints, say 0x98fe985354ab06f2347ac4503f1e24 and 0x42e1f3054ca7432f606ba453589ef89? It should return 0xa363ce3b6849f307be2044b2742ebd44. They have to be very large, so that no parts are zero and 128bit, so the legalizations actually get used on 64bit archs.

Tested using:

fn main() {
    println!("{:x}", 0x98fe985354ab06f2347ac4503f1e24u128.wrapping_mul(0x42e1f3054ca7432f606ba453589ef89u128));
}

Note: I just typed some random numbers and letters to generate those numbers.

[ryzokuken]

@bjorn3 I added a test, but it'll need both iconst.I128 (non existent, but should be fairly straightforward after #1081) and icmp_imm.I128 (#1081) in order to pass...

[bjorn3]

iconst.i64 + iconcat.i128 could be used instead of iconst.i128 for now.

[ryzokuken]

@bjorn3 FAIL filetests/isa/x86/imul-i128.clif: 8: i128 is not a valid typevar for iconcat 😅

[bjorn3]

It will probably work when leaving .i128 out.

[ryzokuken]

@bjorn3 the current test has issues: FAIL filetests/isa/x86/imul-i128.clif: 13: Too many hexadecimal digits. I guess it'll work once we have landed #1081.

[bjorn3]

Immediate values are limited to 64bit as 128bit would drastically increase memory usage. You could use the non imm variant of icmp instead.

[ryzokuken]

@bjorn3 looks like this will now work as soon as icmp is merged. Current error:

FAIL filetests/isa/x86/imul-i128.clif: run(%test_imul_i128):
Verifier errors:
- inst11: v5 is a ghost value used by a real [Op1ret#c3] instruction

[bnjbvr]

Thanks, looking nice!