Cranelift: can't legalize sload32

[whitequark]

I've minimized a test case that currently fails on 32-bit platforms:

target i686

function u0:1516(i32) -> i32 system_v {
block0(v0: i32):
    v1 = load.i32 notrap aligned readonly v0
    ;v2 = load.i32 v1
    ;v3 = sextend.i64 v2
    v3 = sload32 v1
    v4, v5 = isplit v3
    return v4
}

I've tried writing a legalizer for it that would produce the commented instructions but it doesn't do anything (same lack of result for expand, widen, narrow). What gives?

--- a/cranelift/codegen/meta/src/shared/legalize.rs
+++ b/cranelift/codegen/meta/src/shared/legalize.rs
@@ -107,6 +107,7 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
     let sdiv_imm = insts.by_name("sdiv_imm");
     let select = insts.by_name("select");
     let sextend = insts.by_name("sextend");
+    let sload32 = insts.by_name("sload32");
     let sshr = insts.by_name("sshr");
     let sshr_imm = insts.by_name("sshr_imm");
     let srem = insts.by_name("srem");
@@ -213,6 +214,14 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
     // embedded as part of arguments), so use a custom legalization for now.
     narrow.custom_legalize(iconst, "narrow_iconst");
 
+    narrow.legalize(
+        def!(a = sload32.I64(flags, ptr, offset)),
+        vec![
+            def!(b = load.I32(flags, ptr, offset)),
+            def!(a = sextend.I64(b)),
+        ]
+    );
+
     for &(ty, ty_half) in &[(I128, I64), (I64, I32)] {
         let inst = uextend.bind(ty).bind(ty_half);
         narrow.legalize(

[github-actions]

Subscribe to Label Action

cc @bnjbvr

Details

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

• bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[whitequark]

cc @iximeow perhaps?

[bjorn3]

It should probably be in narrow. narrow is used for types bigger than the native pointer size:

wasmtime/cranelift/codegen/meta/src/isa/x86/mod.rs

Lines 40 to 59 in 26ee986

	x86_32.legalize_monomorphic(expand_flags);
	x86_32.legalize_default(x86_narrow);
	x86_32.legalize_type(B1, expand_flags);
	x86_32.legalize_type(I8, x86_widen);
	x86_32.legalize_type(I16, x86_widen);
	x86_32.legalize_type(I32, x86_expand);
	x86_32.legalize_value_type(ReferenceType(R32), x86_expand);
	x86_32.legalize_type(F32, x86_expand);
	x86_32.legalize_type(F64, x86_expand);
	x86_64.legalize_monomorphic(expand_flags);
	x86_64.legalize_default(x86_narrow);
	x86_64.legalize_type(B1, expand_flags);
	x86_64.legalize_type(I8, x86_widen);
	x86_64.legalize_type(I16, x86_widen);
	x86_64.legalize_type(I32, x86_expand);
	x86_64.legalize_type(I64, x86_expand);
	x86_64.legalize_value_type(ReferenceType(R64), x86_expand);
	x86_64.legalize_type(F32, x86_expand);
	x86_64.legalize_type(F64, x86_expand);

[whitequark]

Sure, I tried that first, it doesn't work there either. Or am I doing it wrong?

[abrown]

Looks right to me; what is the error you are getting when you run the snippet?

[whitequark]

what is the error you are getting when you run the snippet?

There is no error. It just doesn't legalize anything. The output is the same as if I don't apply the patch.

[abrown]

Oh, I see now. What about this theory: v1 = load.i32 ... makes v1 an i32 but your legalization is for a = sload32.I64(flags, ptr, offset)--so maybe the legalization is looking for an i64 but the snippet provides an i32? Or something like along those lines?

[whitequark]

@abrown Sorry, my original minimized testcase was not minimized enough and so it was a bit misleading. Take a look at this:

target i686

function u0:1516(i32) -> i32 system_v {
block0(v0: i32):
    ;v1 = load.i32 v0
    ;v2 = sextend.i64 v1
    v2 = sload32 v0
    v3, v4 = isplit v2
    return v3
}

Does this illustrate the problem better?

[bjorn3]

+    narrow.legalize(
+        def!(a = sload32.I64(flags, ptr, offset)),
+        vec![
+            def!(b = load.I32(flags, ptr, offset)),
+            def!(a = sextend.I64(b)),
+        ]
+    );

What @abrown is talking about is that the I64 part of sload32.I64 maybe forces ptr to be an i64, and not the output value.

[whitequark]

Oh... I totally misunderstood how it works. Sorry!

[bjorn3]

Nope

wasmtime/cranelift/wasm/src/code_translator.rs

Line 1819 in 01f46d0

let (load, dfg) = builder.ins().Load(opcode, result_ty, flags, offset, base);

it seems that the I64 part is really the output type and not the pointer type.

[whitequark]

Bizarrely, it works for stores:

    narrow.legalize(
        def!(istore32.I64(flags, a, ptr, offset)),
        vec![
            def!((al, ah) = isplit(a)),
            def!(store.I32(flags, al, ptr, offset)),
        ]
    );

target i686

function u0:0(i32, i32, i32) {
block0(v0: i32, v1: i32, v2: i32):
    v3 = iconcat v1, v2
    istore32.i64 v3, v0
    return
}

[whitequark]

Ahh I know what happens. The problem is that isplit.i64 is not legal and so the legalizer never actually looks at sload32.i64 at all. It looks like it's not easily possible to encode that in the meta language, how would I proceed?