From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] x86/emul: only emulate possibly operand sizes for POPA Date: Thu, 08 Nov 2012 07:48:23 +0000 Message-ID: References: <509B6EB002000078000A71CB@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <509B6EB002000078000A71CB@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 08/11/2012 07:34, "Jan Beulich" 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