Stream: cranelift

Topic: Fixing issue 6185

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
; }

kmeakin (Apr 11 2023 at 21:36):

Are integer constants sign extended during parsing or something?

Jamey Sharp (Apr 11 2023 at 21:36):

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

Jamey Sharp (Apr 11 2023 at 21:38):

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

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)

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?

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

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.

kmeakin (Apr 11 2023 at 21:46):

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

Jamey Sharp (Apr 11 2023 at 21:46):

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

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.

Jamey Sharp (Apr 11 2023 at 21:54):

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

• Make the CLIF parser zero-extend Imm64 according to the type of the iconst instruction.
• Make the CLIF verifier check that the upper bits are zero, in case an ISLE rule or a frontend other than the CLIF parser gets this wrong.
• Make ISLE rules get constant values through a helper that either zero-extends or sign-extends the value according to the iconst instruction's type. The calling rule needs to specify whether it's expecting a signed or unsigned number since we don't distinguish between those in the CLIF type system.

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…

kmeakin (Apr 11 2023 at 22:00):

zero-extending in the parser seems like the simplest solution

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.

kmeakin (Apr 11 2023 at 22:03):

I'll see what I can do

kmeakin (Apr 11 2023 at 22:09):

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

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.

Last updated: Nov 22 2024 at 16:03 UTC