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

[David Gibson]

On Fri, May 27, 2016 at 08:43:15AM +0100, Mark Cave-Ayland wrote:
> On 27/05/16 02:36, David Gibson wrote:
> > On Thu, May 26, 2016 at 07:54:46AM +0100, Mark Cave-Ayland wrote:
> >> On 19/05/16 13:11, Talha Imran wrote:
> >>
> >> (Adding CC to David as maintainer)
> >>
> >>> 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 <address@hidden>
> >>> ---
> >>>  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);
> >>>
> >>
> >>
> >> ATB,
> > 
> > Sorry, I'm not sure what that means.
> 
> Oh that's just my standard signature - no need to worry about that
> :)

Ah, ok.

Can I get a R-b from you on this one, or is this not your bailiwick?

-- 
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

signature.asc
Description: PGP signature