Stream: git-wasmtime

Topic: wasmtime / PR #6086 x64: emit_cmp: use x64_test for compa...

Wasmtime GitHub notifications bot (Mar 22 2023 at 11:21):

meithecatte opened PR #6086 from emit-cmp-test to main:

This implements the first part of the improvement described in #5869.

The PR template suggests I assign someone for review, but I have no idea who'd be best suited for this.

cc @elliottt

Wasmtime GitHub notifications bot (Mar 23 2023 at 11:03):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Mar 23 2023 at 11:03):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Mar 23 2023 at 11:03):

abrown created PR review comment:

As a note for others: I was a bit nervous about reverse here and ended up looking up whether inverse is actually the right thing — we can see from rule 1, though, that reverse is used there as well.

Wasmtime GitHub notifications bot (Mar 23 2023 at 15:08):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 23 2023 at 15:08):

jameysharp created PR review comment:

Yeah, I'm frequently confused between inverse and reverse too. I think both names are correct descriptions of what they do, but maybe later we can come up with alternate names that are less confusing. Maybe "negate" instead of "inverse", and "swap" instead of "reverse", for example.

Wasmtime GitHub notifications bot (Mar 23 2023 at 15:21):

meithecatte submitted PR review.

Wasmtime GitHub notifications bot (Mar 23 2023 at 15:21):

meithecatte created PR review comment:

@jameysharp Good idea! I think these are much better names.

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

alexcrichton submitted PR review.

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

alexcrichton created PR review comment:

To fix the issue on CI, I think this should work?

(let (
    (size OperandSize (raw_operand_size_of_type ty))
    (a Gpr (put_in_gpr a))
  )
  (icmp_cond_result (x64_test size a a) cc))

Basically by forcing a into a register earlier it's not separately converted to both GprMem and Gpr

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

jameysharp submitted PR review.

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

jameysharp created PR review comment:

Yes, I think that's the right fix!

Wasmtime GitHub notifications bot (Mar 24 2023 at 08:25):

meithecatte updated PR #6086 from emit-cmp-test to main.

Wasmtime GitHub notifications bot (Mar 24 2023 at 08:26):

meithecatte submitted PR review.

Wasmtime GitHub notifications bot (Mar 24 2023 at 08:26):

meithecatte created PR review comment:

Yup, that works. I added a filetest that triggers this assertion, too.

Wasmtime GitHub notifications bot (Mar 27 2023 at 15:38):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 27 2023 at 16:17):

jameysharp merged PR #6086.

Last updated: Dec 23 2024 at 12:05 UTC