x86: Force result of Icomp to be in a register

[lthls]

Fixes #11803.
I've made the PR against 4.14, as the bug report only mentioned 32-bit native code which is not supported on trunk, but I think this should be considered for trunk too.
I've checked that the issue reported in #11803 disappears with this patch.

[xavierleroy]

Looks good to me. A suggestion for shorter code below. To be copy-pasted to i386 if you adopt it.

An alternative that I considered is to force the result of Icomp to be in the RAX register, and emit code like this:

  xor rax, rax
  cmp ...
  setxx al

This avoids a partial register stall on writing to AL, which could be good for performance. However, if the result cannot be held in RAX, an extra move will be generated by the register allocator.

[xavierleroy]

makereg is a no-op if the argument is already a register, and there's makeregs to handle arrays of regs. Proposed simplification:

Suggested change

	(* The result must be a register *)
	let res =
	if stackp res.(0)
	then [|self#makereg res.(0)|]
	else res
	in
	(* The result must be a register (PR#11083) *)
	let res = self#makeregs res in

[lthls]

I didn't think of calling makereg even in the register case, but I didn't use makeregs because it wasn't exported. I assume that there's no harm in exporting it though, so I've pushed a patch with your suggestion.

[xavierleroy]

Similar suggestion:

Suggested change

	| Iintop_imm(Icomp _, _) ->
	(* The result must be in a register *)
	if stackp res.(0)
	then (arg, [|self#makereg res.(0)|])
	else (arg, res)
	| Iintop_imm(Icomp _, _) ->
	(* The result must be in a register (PR#11083) *)
	(arg, self#makeregs res)

[stedolan]

An alternative that I considered is to force the result of Icomp to be in the RAX register, and emit code like this:

That code sequence looks like it's probably better (as you say, avoiding the partial reg stall is good). But the xor should be after the cmp, because one of the arguments can arrive in RAX.

[xavierleroy]

But the xor should be after the cmp, because one of the arguments can arrive in RAX.

Good point. But the xor changes the condition codes :-) A move should do, however:

  cmp ...
  movl $0, eax
  setxx al

[smuenzel]

  cmp ...
  movl $0, eax
  setxx al

I believe that movl 0 is not one of the zeroing idioms recognized by most processors, meaning that the instruction has to be executed by the processor. xor is eliminated by rename, so it does not consume execution resources

[stedolan]

But the xor changes the condition codes :-)

Hah, true! Perhaps we should have Emit choose xor; cmp; setcc when the result does not alias the input, and cmp; mov; setcc when it does?

Incidentally, on amd64 there's no need to have a mov at the end, since setcc can reach the low 8 bits of every integer register. (This is not true on i386 where the 8-bit registers are weird)

[smuenzel]

There is some useful discussion here: https://stackoverflow.com/questions/33666617/what-is-the-best-way-to-set-a-register-to-zero-in-x86-assembly-xor-mov-or-and (see the section "Things are more complicated when you don't want to xor before a flag-setting instruction. ")

[xavierleroy]

Looks good to me. Thanks!

[xavierleroy]

OK, this is merged on 4.14. Once CI is happy we'll backport to trunk.

[xavierleroy]

Cherry-picked to trunk: c314da5

[xavierleroy]

@stedolan @smuenzel : I pushed the quick fix for issue #11803, but you're welcome to try and improve the x86-64 code generated for Icomp instructions. In my opinion, it's not performance-critical, but it's a good reminder of how awful the x86 ISA is :-)