Stream: git-wasmtime

Topic: wasmtime / Issue #1747 Cranelift: can't legalize sload32

Wasmtime GitHub notifications bot (May 22 2020 at 11:26):

whitequark opened Issue #1747:

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");


+    expand.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(

Wasmtime GitHub notifications bot (May 22 2020 at 11:26):

whitequark labeled Issue #1747:

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");


+    expand.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(

Wasmtime GitHub notifications bot (May 22 2020 at 11:26):

whitequark labeled Issue #1747:

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");


+    expand.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(

Wasmtime GitHub notifications bot (May 22 2020 at 11:26):

github-actions[bot] commented on Issue #1747:

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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (May 22 2020 at 11:28):

whitequark commented on Issue #1747:

cc @iximeow perhaps?

Wasmtime GitHub notifications bot (May 22 2020 at 11:37):

bjorn3 commented on Issue #1747:

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

Wasmtime GitHub notifications bot (May 22 2020 at 11:38):

bjorn3 edited a comment on Issue #1747:

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

https://github.com/bytecodealliance/wasmtime/blob/26ee986c2f99bb339f171267731f9108890044ab/cranelift/codegen/meta/src/isa/x86/mod.rs#L40-L59

Wasmtime GitHub notifications bot (May 22 2020 at 11:40):

whitequark edited Issue #1747:

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(

Wasmtime GitHub notifications bot (May 22 2020 at 11:40):

whitequark commented on Issue #1747:

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

Wasmtime GitHub notifications bot (May 27 2020 at 16:52):

abrown commented on Issue #1747:

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

Wasmtime GitHub notifications bot (May 27 2020 at 22:52):

whitequark commented on Issue #1747:

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

There is no error. It just doesn't legalize anything.

Wasmtime GitHub notifications bot (May 27 2020 at 22:52):

whitequark edited a comment on Issue #1747:

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.

Wasmtime GitHub notifications bot (May 28 2020 at 15:53):

abrown commented on Issue #1747:

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?

Wasmtime GitHub notifications bot (May 29 2020 at 10:40):

whitequark commented on Issue #1747:

@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?

Wasmtime GitHub notifications bot (May 29 2020 at 11:51):

bjorn3 commented on Issue #1747:

diff + 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 forces ptr to be an i64, not the output value.

Wasmtime GitHub notifications bot (May 29 2020 at 11:51):

bjorn3 edited a comment on Issue #1747:

diff + 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, not the output value.

Wasmtime GitHub notifications bot (May 29 2020 at 11:52):

bjorn3 edited a comment on Issue #1747:

diff + 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.

Wasmtime GitHub notifications bot (May 29 2020 at 11:53):

whitequark commented on Issue #1747:

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

Wasmtime GitHub notifications bot (May 29 2020 at 11:55):

bjorn3 commented on Issue #1747:

Nope

https://github.com/bytecodealliance/wasmtime/blob/01f46d02383be038465d86d5c2e22e2d2454d3ec/cranelift/wasm/src/code_translator.rs#L1819

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

Wasmtime GitHub notifications bot (May 29 2020 at 17:33):

whitequark commented on Issue #1747:

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
}

Wasmtime GitHub notifications bot (May 29 2020 at 17:41):

whitequark commented on Issue #1747:

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?

Wasmtime GitHub notifications bot (May 29 2020 at 19:20):

bjorn3 commented on Issue #1747:

if pos.func.dfg.value_type(args[0]) == ir::types::I64 {

this is an excerpt from the sload32 legalization. It seems that it does check the pointer type.

Wasmtime GitHub notifications bot (May 29 2020 at 19:21):

whitequark commented on Issue #1747:

That seems like a bug then? But I'm not sure where.

Wasmtime GitHub notifications bot (May 29 2020 at 19:31):

bjorn3 commented on Issue #1747:

It seems that the ctrl typevar of sload32 is actually the pointer size. The output type is always i64. This means that you should use sload32.i32 when you have a 32bit pointer and sload32.i64 when you have a 64bit pointer.

let iAddr = &TypeVar::new(
    "iAddr",
    "An integer address type",
    TypeSetBuilder::new().ints(32..64).build(),
);
// ...
let p = &Operand::new("p", iAddr);
// ...
let iExt32 = &TypeVar::new(
    "iExt32",
    "An integer type with more than 32 bits",
    TypeSetBuilder::new().ints(64..64).build(),
);
let x = &Operand::new("x", iExt32);
let a = &Operand::new("a", iExt32);
// ...
ig.push(
    Inst::new(
        "sload32",
        r#"
    Load 32 bits from memory at ``p + Offset`` and sign-extend.

    This is equivalent to ``load.i32`` followed by ``sextend``.
    "#,
        &formats.load,
    )
    .operands_in(vec![MemFlags, p, Offset])
    .operands_out(vec![a])
    .can_load(true),
);

Only p and a are values. p can be i32 or i64, but a can only be i64.

Wasmtime GitHub notifications bot (May 29 2020 at 19:33):

bjorn3 commented on Issue #1747:

Using sload.I32 in the legalization and moving it to expand fixes it.

Wasmtime GitHub notifications bot (May 29 2020 at 19:40):

whitequark commented on Issue #1747:

Thanks!

Wasmtime GitHub notifications bot (Oct 21 2020 at 06:01):

whitequark commented on Issue #1747:

I believe #1794 has fixed but didn't close this issue because of some CI weirdness.

Wasmtime GitHub notifications bot (Oct 21 2020 at 06:01):

whitequark closed Issue #1747:

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(

Last updated: Oct 23 2024 at 20:03 UTC