Stream: cranelift

Topic: Fixing issue 6185


view this post on Zulip kmeakin (Apr 11 2023 at 21:35):

I'm trying to solve https://github.com/bytecodealliance/wasmtime/issues/6185
I can only reproduce the bug if umax is written as -1. If it is written in hexadecimal (0xFFFF_FFFF), the bug does not reproduce:

test optimize precise-output
set opt_level=speed
target x86_64

function %icmp_ule_umax(i32) -> i8 {
block0(v0: i32):
    v1 = iconst.i32 0xFFFF_FFFF
    v2 = icmp ule v0, v1
    return v2
}

; function %icmp_ule_umax(i32) -> i8 fast {
; block0(v0: i32):
;     v3 = iconst.i8 1
;     return v3  ; v3 = 1
; }

function %icmp_ule_minus_1(i32) -> i8 {
block0(v0: i32):
    v1 = iconst.i32 -1
    v2 = icmp ule v0, v1
    return v2
}

; function %icmp_ule_minus_1(i32) -> i8 fast {
; block0(v0: i32):
;     v1 = iconst.i32 -1
;     v2 = icmp ule v0, v1  ; v1 = -1
;     return v2
; }

function %icmp_eq_umax_minus_1(i32) -> i8 {
block0(v0: i32):
    v1 = iconst.i32 -1
    v2 = iconst.i32 0xFFFF_FFFF
    v3 = icmp eq v1, v2
    return v3
}

; function %icmp_eq_umax_minus_1(i32) -> i8 fast {
; block0(v0: i32):
;     v4 = iconst.i8 1
;     return v4  ; v4 = 1
; }

function %foo(i32) -> i32, i32 {
block0(v0: i32):
    v1 = iconst.i32 -1
    v2 = iconst.i32 0xFFFF_FFFF
    return v1, v2
}

; function %foo(i32) -> i32, i32 fast {
; block0(v0: i32):
;     v1 = iconst.i32 -1
;     v2 = iconst.i32 0xffff_ffff
;     return v1, v2  ; v1 = -1, v2 = 0xffff_ffff
; }
Fuzzing has found that this module: (module (func $f (param i32) (result i32) (local i64) local.get 0 i32.const -1 i32.le_u ) (func $s (call $f (i32.const 0)) if return end unreachable ) (start $s)...

view this post on Zulip kmeakin (Apr 11 2023 at 21:36):

Are integer constants sign extended during parsing or something?

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:36):

Yeah, that's due to a different issue that we need to clean up :sweat_smile:

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:38):

Specifically, https://github.com/bytecodealliance/wasmtime/issues/5700 and https://github.com/bytecodealliance/wasmtime/issues/3059

Feature In verification conversations, it’s come up that we don’t have a precise semantics for the underlying u64 when a CLIF iconst has a narrow type (e.g., i8). Should the narrow constant be sign...
Feature As discussed in #3056 (comment) we allow const instructions with values that are way too large for their type (i.e. 0xff00 for an i8). We should reject these modules in the CLIF validator. ...

view this post on Zulip kmeakin (Apr 11 2023 at 21:38):

What I find extra confusing is that both constants are considered equal by GVN/cprop (icmp_eq_umax_minus_1) but not by the ISLE pattern matcher (icmp_ule_minus_1)

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:40):

I haven't looked closely at those cases. What leads you to think that GVN behaves differently than ISLE on the same constants?

view this post on Zulip kmeakin (Apr 11 2023 at 21:42):

This function:

function %icmp_eq_umax_minus_1(i32) -> i8 {
block0(v0: i32):
    v1 = iconst.i32 -1
    v2 = iconst.i32 0xFFFF_FFFF
    v3 = icmp eq v1, v2
    return v3
}

is optimized to return 1. Either by GVN recognising that v1 and v2 are the same, and then a rewrite rule for icmp eq x x => 1. Or by cprop

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:43):

Ah, got it. I think that's due to cprop, where I believe the rules specifically zero-extend all constants.

view this post on Zulip kmeakin (Apr 11 2023 at 21:46):

So the problem is that -1 and 0xFFFF_FFFF are represented differently in Imm64?

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:46):

Yeah. They shouldn't be, but they are.

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:48):

The parser reads both numbers as an i64 (more or less) and stuffs the result into Imm64, without any consideration for the type it's supposed to be constructing.

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:54):

There are several fixes we want to make around this issue:

view this post on Zulip Jamey Sharp (Apr 11 2023 at 21:55):

Nobody has started working on any of those fixes though; there are always plenty of other things to work on…

view this post on Zulip kmeakin (Apr 11 2023 at 22:00):

zero-extending in the parser seems like the simplest solution

view this post on Zulip Jamey Sharp (Apr 11 2023 at 22:01):

It's good, yeah, but not a complete fix. Still, I'd definitely take that in a PR even without the other fixes.

view this post on Zulip kmeakin (Apr 11 2023 at 22:03):

I'll see what I can do

view this post on Zulip kmeakin (Apr 11 2023 at 22:09):

hmm...Maybe remove Imm64 and replace all uses with UImm64 and explicit sign-extensions where necessary?

view this post on Zulip Jamey Sharp (Apr 11 2023 at 22:38):

That's definitely worth considering. I think fitzgen's suggestion at https://github.com/bytecodealliance/wasmtime/issues/5700#issuecomment-1421260557 is a more comprehensive version of that approach.

Feature In verification conversations, it’s come up that we don’t have a precise semantics for the underlying u64 when a CLIF iconst has a narrow type (e.g., i8). Should the narrow constant be sign...

Last updated: Dec 23 2024 at 13:07 UTC