Cranelift: support scalar types for min/max instructions

[DGriffin91]

Using cranelift 0.76, the following results in this error message: "'Unsupported type for imin instruction: i64', [...]\cranelift-codegen-0.76.0\src\isa\x64\lower.rs:2353:17"

function u0:0() -> f64 windows_fastcall {
block0:
    v0 = f64const 0.0
    v1 = iconst.i64 1
    v2 = iconst.i64 2
    v3 = imin v1, v2
    v4 = fcvt_from_sint.f64 v3
    return v4
}

Am I using imin incorrectly?

[cfallin]

Currently the imin instruction is only supported for vector types, not scalars. It's a part of a larger issue in which our backends support the particular combinations of instructions and types that are generated by our Wasm frontend, but still have implementation gaps for the fully general opcode x type matrix. Sorry about that! I'd be happy to point out the place where this would be implemented if you're interested in possibly contributing a patch, and in the meantime I'll go ahead and rename this issue to track the missing impl.

Thanks again for the report!

[DGriffin91]

Thanks for the explanation!

Sure! I would be interested in seeing where it would be implemented.

[afonso360]

That particular assert is being triggered when we try to lower the CLIF Opcode into a x86 machine instruction. That is done in the x64 backend lower.rs file, specifically line 2343.

The first step would be figuring out what assembly we want to generate for each instruction. rustc lowers this as a mov, cmp and cmovg. I'm not too familiar with x86 to be sure if this is the best instruction sequence, but maybe someone can comment on that later.

Reading the x64 backend a bit, it looks like we have helpers for emitting those instructions. emit_cmp and emit_cmoves. For mov, you can call Inst::gen_move which should generate the right thing. Between those three functions you should be able to generate assembly sequence above.

While doing all these changes an easy way to test them is to build a CLIF test file, and run it using clif-util. Looking at our testsuite, it looks like we don't have any test files for imin, so its also a good idea to commit the test file that you generate when you implement this!

A simple testfile for imin.i64 could be:

test run
target x86_64 machinst

function %imin_i64(i64, i64) -> i64 {
block0(v0: i64,v1: i64):
    v2 = imin v0, v1
    return v2
}
; run: %imin_i64(0, 0) == 0
; run: %imin_i64(0, 1) == 0
; run: %imin_i64(-1, 0) == -1

Although you'd probably want to add tests for whatever other types / instructions you are implementing. You can then run this test file using cargo run test ./test-file.clif, which compiles the function and runs the tests in the comments.

If you want to view the disassembly of the code you are generating you can run cargo run compile --target x86_64 -D ./test-file.clif

Hope this helps, good luck!

[akirilov-arm]

This issue also looks like a duplicate of #1505.

[afonso360]

This seems to have been fixed in #3748 for types i64 and smaller, we are still crashing on i128.