Re: [Qemu-devel] [Qemu-ppc] [PATCH] target-ppc/fpu_helper: Fix efscmp* instructions handling

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3437 bytes --]

On Thu, May 19, 2016 at 05:11:35PM +0500, Talha Imran wrote:
> With specification at hand from the reference manual from Freescale
> http://cache.nxp.com/files/32bit/doc/ref_manual/SPEPEM.pdf , I have found a fix
> to efscmp* instructions handling in QEMU.
> 
> efscmp* instructions in QEMU set crD (Condition Register nibble) values as 
> (0b0100 << 2) = 0b10000 (consider the HELPER_SINGLE_SPE_CMP macro which left 
> shifts the value returned by efscmp* handler by 2 bits). A value of 0b10000 is
> not correct according the to the reference manual.
> 
> The reference manual expects efscmp* instructions to return a value of 0bx1xx.
> Please find attached a patch which disables left shifting in
> HELPER_SINGLE_SPE_CMP macro. This macro is used by efscmp* and efstst*
> instructions only. efstst* instruction handlers, in turn, call efscmp* handlers
> too.
> 
> *Explanation:*
> Traditionally, each crD (condition register nibble) consist of 4 bits, which is
> set by comparisons as follows:
> crD = W X Y Z
> where
> W = Less than
> X = Greater than
> Y = Equal to
> 
> However, efscmp* instructions being a special case return a binary result.
> (efscmpeq will set the crD = 0bx1xx iff when op1 == op2 and 0bx0xx otherwise;
> i.e. there is no notion of different crD values based on Less than, Greater
> than and Equal to).
> 
> This effectively means that crD will store a "Greater than" comparison result
> iff efscmp* instruction comparison is TRUE. Compiler exploits this feature by
> checking for "Branch if Less than or Equal to" (ble instruction) OR "Branch if
> Greater than" (bgt instruction) for Branch if FALSE OR Branch if TRUE
> respectively after an efscmp* instruction. This can be seen in a assembly code
> snippet below:
> 
> 27          if (__real__ x != 3.0f || __imag__ x != 4.0f)
> 10000498:   lwz r10,8(r31)
> 1000049c:   lis r9,16448
> 100004a0:   efscmpeq cr7,r10,r9
> 100004a4:   ble- cr7,0x100004b8 <bar+60>  //jump to abort() call
> 100004a8:   lwz r10,12(r31)
> 100004ac:   lis r9,16512
> 100004b0:   efscmpeq cr7,r10,r9
> 100004b4:   bgt- cr7,0x100004bc <bar+64>  //skip abort() call
> 28            abort ();
> 100004b8:   bl 0x10000808 <abort>
> 
> Signed-off-by: Talha Imran <talha_imran@mentor.com>

Does this patch supersede the earlier patch you posted for efcmp
instructions on e500v1?  Or is it in addition to that patch?

> ---
>  target-ppc/fpu_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> index b67ebca..6fd56a8 100644
> --- a/target-ppc/fpu_helper.c
> +++ b/target-ppc/fpu_helper.c
> @@ -1442,7 +1442,7 @@ static inline uint32_t efststeq(CPUPPCState *env, uint32_t op1, uint32_t op2)
>  #define HELPER_SINGLE_SPE_CMP(name)                                     \
>      uint32_t helper_e##name(CPUPPCState *env, uint32_t op1, uint32_t op2) \
>      {                                                                   \
> -        return e##name(env, op1, op2) << 2;                             \
> +        return e##name(env, op1, op2);                                  \
>      }
>  /* efststlt */
>  HELPER_SINGLE_SPE_CMP(fststlt);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]