x64: emit_cmp: use x64_test for comparisons with 0

[meithecatte]

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

[jameysharp]

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

[abrown]

This change looks good to me! It is a bit of a micro-optimization, but every bit helps (or "byte" in this case). I verified in Intel's optimization manual that this change will help both with macrofusion (merge two instructions into a single micro-op) and code size:

Assembly/Compiler Coding Rule 17. (M impact, ML generality) Employ macrofusion where
possible using instruction pairs that support macrofusion. Prefer TEST over CMP if possible. Use unsigned variables and unsigned jumps when possible. Try to logically verify that a variable is nonnegative at the time of comparison. Avoid CMP or TEST of MEM-IMM flavor when possible. However, do not add other instructions to avoid using the MEM-IMM flavor.

Beyond the MEM-IMM issue, what is going on here is that CMP will not macrofuse with a subset of conditional jumps but TEST will macrofuse with all of them (see section 3.4.2.2, Optimizing for Macrofusion).

Assembly/Compiler Coding Rule 36. (ML impact, M generality) Use the TEST instruction instead of AND when the result of the logical AND is not used. This saves µops in execution. Use a TEST of a register with itself instead of a CMP of the register to zero, this saves the need to encode the zero and saves encoding space. Avoid comparing a constant to a memory operand. It is preferable to load the memory operand and compare the constant to a register.

Thanks!

[abrown]

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.

[jameysharp]

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.

[meithecatte]

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

[meithecatte]

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?

[meithecatte]

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?

[cfallin]

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.

[alexcrichton]

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

[jameysharp]

Yes, I think that's the right fix!

[meithecatte]

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

[jameysharp]

This looks great, thank you, and an extra thank you for adding the file test.