Stream: cranelift

Topic: bnot on b1s

Nathan Ringo (Dec 31 2020 at 01:01):

I have some code like

v7 = icmp eq v5, v6
v8 = bnot v7
...
v13 = select v8, v9, v11

This compiles to

cmp edx, eax
sete al
not eax
...
test al, al
jnz ...

This seems incorrect: if the code snippet starts with rax=0, rdx=0, then it proceeds as:

cmp edx, eax ; ZF=1
sete al ; rax=1
not eax ; rax=0xfffffffe
...
test al, al ; zf=0
jnz ... ; taken!

I'd imagine it should generate an and after the not, would would execute as:

cmp edx, eax ; ZF=1
sete al ; rax=1
not eax ; rax=0xfffffffe
and eax, 1 ; rax=0
...
test al, al ; zf=1
jnz ... ; not taken

bjorn3 (Dec 31 2020 at 06:31):

Or sete eax.

Julian Seward (Dec 31 2020 at 08:26):

Fixing this in the instruction selectors is the wrong place. Better would be for CLIF level rewriting to turn it into

v7 = icmp eq v5, v6
v13 = select v7, v11, v9

bjorn3 (Dec 31 2020 at 08:56):

No, fixing this in the instruction selectors is not the wrong place. This is causing valid clif ir to be miscompiled.

bjorn3 (Dec 31 2020 at 08:57):

Clif ir shouldn't need a transformation first to be correctly lowered to vcode. It is fine to do the rewriting you suggested as optimization, but not as mandatory pass.

Chris Fallin (Dec 31 2020 at 09:15):

@Nathan Ringo and all -- yes, this is a miscompilation; thanks for reporting it! Agree with @bjorn3 that we need to handle this input properly. A b1 has all bits above bit 0 undefined, though, so I think the proper fix is in the select lowering, not the icmp or bnot; e.g. the select could be test al, 1 / jnz ...

Chris Fallin (Dec 31 2020 at 09:15):

I can get to this first thing next week

Chris Fallin (Dec 31 2020 at 09:17):

And, as an additional note, apologies -- the new-backend code paths outside of i32/i64/f32/f64 types are less well-tested, as we've been finding with the cg_clif bringup as well, since we started with the Wasm frontend and mostly have tested with that so far. Hoping to do a full audit of this sort of thing (undefined bits / proper width handling) at some point soon so we're fully robust across all valid CLIF!

Julian Seward (Dec 31 2020 at 11:04):

@bjorn3 oh, sorry. My bad. I should have had more coffee before typing ..

Nathan Ringo (Dec 31 2020 at 18:50):

@Chris Fallin Okay, thanks!

Chris Fallin (Jan 05 2021 at 01:00):

PR to fix this: https://github.com/bytecodealliance/wasmtime/pull/2546

Last updated: Oct 23 2024 at 20:03 UTC