Stream: git-wasmtime

Topic: wasmtime / issue #6086 x64: emit_cmp: use x64_test for co...

Wasmtime GitHub notifications bot (Mar 23 2023 at 01:30):

jameysharp commented on issue #6086:

Neat, thanks for picking this up! I think this is correct, but I'd like @elliottt or @abrown to take a look.

Wasmtime GitHub notifications bot (Mar 23 2023 at 16:48):

meithecatte commented on issue #6086:

Hmm, any idea what could be happening with that test failure? Is there some special-case around sinking for cmp that needs to be extended to test?

Wasmtime GitHub notifications bot (Mar 23 2023 at 16:53):

meithecatte commented on issue #6086:

Hmm, this optimization can turn one use at the IR level into two uses at the same instruction, at the opcode level. Perhaps this confuses the sinking logic?

Wasmtime GitHub notifications bot (Mar 23 2023 at 17:04):

cfallin commented on issue #6086:

Hmm, this optimization can turn one use at the IR level into two uses at the same instruction, at the opcode level. Perhaps this confuses the sinking logic?

Yes, we have some subtle invariants here where lowering icmp/fcmp is special: we lower at every use (because otherwise we'd have to spill/restore flags), so we go to special lengths to not allow load sinking. See e.g. #3934. We'd like to come up with a more principled way to enforce this invariant, maybe a "special" lowering context state that knows we're lowering something that can be lowered multiple times; but right now we don't have that. Unfortunately I think this means that this change can't work as-is.

Last updated: Nov 22 2024 at 16:03 UTC