Re: [Qemu-devel] [PATCH 2/9] target-ppc: Fix xscmpodp and xscmpudp instructions

[Bharata B Rao <bharata@linux.vnet.ibm.com>]

On Wed, Nov 23, 2016 at 03:01:18PM +1100, David Gibson wrote:
> On Tue, Nov 22, 2016 at 05:15:58PM +0530, Nikunj A Dadhania wrote:
> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > - xscmpodp & xscmpudp are missing flags reset.
> > - In xscmpodp, VXCC should be set only if VE is 0 for signalling NaN case
> >   and VXCC should be set by explicitly checking for quiet NaN case.
> > - Comparison is being done only if the operands are not NaNs. However as
> >   per ISA, it should be done even when operands are NaNs.
> 
> For my interest, can you explain the difference between ordered and
> unordered comparisons?  I looked at the ISA and mostly just became
> confused.

>From another section of the same ISA doc, I see these description which
makes the distinction between ordered and unordered comparisions a bit
more clear.

Unordered:

"If either of the operands is a NaN, either quiet or signal-
ing, then CR field BF and the FPCC are set to reflect
unordered. If either of the operands is a Signaling NaN,
then VXSNAN is set."

Ordered:

"If either of the operands is a NaN, either quiet or signal-
ing, then CR field BF and the FPCC are set to reflect
unordered. If either of the operands is a Signaling NaN,
then VXSNAN is set and, if Invalid Operation is dis-
abled (VE=0), VXVC is set. If neither operand is a Sig-
naling NaN but at least one operand is a Quiet NaN,
then VXVC is set."
 
> 
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> > ---
> >  target-ppc/fpu_helper.c | 41 +++++++++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target-ppc/fpu_helper.c b/target-ppc/fpu_helper.c
> > index d3741b4..3027003 100644
> > --- a/target-ppc/fpu_helper.c
> > +++ b/target-ppc/fpu_helper.c
> > @@ -2410,29 +2410,38 @@ void helper_##op(CPUPPCState *env, uint32_t opcode)                      \
> >  {                                                                        \
> >      ppc_vsr_t xa, xb;                                                    \
> >      uint32_t cc = 0;                                                     \
> > +    bool vxsnan_flag = false, vxvc_flag = false;                         \
> >                                                                           \
> > +    helper_reset_fpstatus(env);                                          \
> >      getVSR(xA(opcode), &xa, env);                                        \
> >      getVSR(xB(opcode), &xb, env);                                        \
> >                                                                           \
> > -    if (unlikely(float64_is_any_nan(xa.VsrD(0)) ||                       \
> > -                 float64_is_any_nan(xb.VsrD(0)))) {                      \
> > -        if (float64_is_signaling_nan(xa.VsrD(0), &env->fp_status) ||     \
> > -            float64_is_signaling_nan(xb.VsrD(0), &env->fp_status)) {     \
> > -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXSNAN, 0);       \
> > -        }                                                                \
> > -        if (ordered) {                                                   \
> > -            float_invalid_op_excp(env, POWERPC_EXCP_FP_VXVC, 0);         \
> > +    if (float64_is_signaling_nan(xa.VsrD(0), &env->fp_status) ||         \
> > +        float64_is_signaling_nan(xb.VsrD(0), &env->fp_status)) {         \
> > +        vxsnan_flag = true;                                              \
> > +        cc = 1;                                                          \
> > +        if (fpscr_ve == 0 && ordered) {                                  \
> > +            vxvc_flag = true;                                            \
> >          }                                                                \
> > +    } else if ((float64_is_quiet_nan(xa.VsrD(0), &env->fp_status) ||     \
> > +                float64_is_quiet_nan(xb.VsrD(0), &env->fp_status))       \
> > +               && ordered) {                                             \
> >          cc = 1;                                                          \
> 
> Since you're basically rewriting this, could you please change it to
> use symbolic constants for the CC bits, which will make it easier to
> follow.

Sure will do.

Regards,
Bharata.