Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA

[Keir Fraser <keir.xen@gmail.com>]

On 08/11/2012 07:34, "Jan Beulich" <JBeulich@suse.com> wrote:

>> Would prefer:
>>  if ( op_bytes == 2 )
>>      *(uint16_t *)regs[i] = (uint16_t)dst.val;
>>  else
>>      *regs[i] = dst.val;
>> 
>> Handles the exceptional case immediately after its predicate.
> 
> I had it that way first, but compilers tend to prefer (in terms of
> static branch prediction) the if() body over the else one. Doesn't
> matter that much here of course, but I'm generally trying to
> follow such guidelines even in non performance critical paths so
> that in case code gets cloned elsewhere it doesn't require extra
> reviewing or adjustment.

Should follow such guidelines where the optimisation matters. I think
shaping code to follow such guidelines all the time, is misguided. I'd
rather have the fractionally more readable version than the
possibly-fractionally faster version.

>> And the cast
>> from uint32_t, and 64b-related comment, are pointless and in fact misleading
>> in the default case, since as you say the instruction is invalid in 64-bit
>> mode.
> 
> And I considered that aspect too: Even if invalid in 64-bit mode, it
> is valid in compatibility mode, and in that case the zero-extension
> makes sense (as does the comment).

I did wonder. The top halves of 64b registers are not used in compatibility
mode. Are their contents at all guaranteed to be
maintained/updated/preserved in any meaningful way across transitions into
and out of compatibility mode? I wasn't aware they were, and in that case
the cast and comment are indeed pointless.

 -- Keir