qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Aleksandar Markovic <aleksandar.m.mail@gmail.com>
To: "Paul A. Clarke" <pc@us.ibm.com>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, david@gibson.dropbear.id.au
Subject: Re: [Qemu-devel] [PATCH] ppc: Three floating point fixes
Date: Sat, 17 Aug 2019 09:53:59 +0200	[thread overview]
Message-ID: <CAL1e-=hAcMMQMP4ecW_AW_3UM1yCjRE+P7QN3ytn2JaZeTFxUg@mail.gmail.com> (raw)
In-Reply-To: <CAL1e-=jy6vggskJ26rTc8dnaqtqCB0SdfpV9p-NvKdjoBk+Vkw@mail.gmail.com>

17.08.2019. 00.59, "Aleksandar Markovic" <aleksandar.m.mail@gmail.com> је
написао/ла:
>
>
> 16.08.2019. 21.28, "Paul A. Clarke" <pc@us.ibm.com> је написао/ла:
> >
> > From: "Paul A. Clarke" <pc@us.ibm.com>
> >
> > - target/ppc/fpu_helper.c:
> >   - helper_todouble() was not properly converting INFINITY from 32 bit
> >   float to 64 bit double.
> >   - helper_todouble() was not properly converting any denormalized
> >   32 bit float to 64 bit double.
> >
> > - GCC, as of version 8 or so, takes advantage of the hardware's
> >   implementation of the xscvdpspn instruction to optimize the following
> >   sequence:
> >     xscvdpspn vs0,vs1
> >     mffprwz   r8,f0
> >   ISA 3.0B has xscvdpspn leaving its result in word 1 of the target
register,
> >   and mffprwz expecting its input to come from word 0 of the source
register.
> >   This sequence fails with QEMU, as a shift is required between those
two
> >   instructions.  However, the hardware splats the result to both word 0
and
> >   word 1 of its output register, so the shift is not necessary.
> >   Expect a future revision of the ISA to specify this behavior.
> >
>
> Hmmm... Isn't this a gcc bug (using undocumented hardware feature), given
everything you said here?
>

Paul, you are touching here a very sensitive area. If I were you, I would
most likely propose a similar patch, given thr info you presented. However,
in general, QEMU is a compiler-agnostic tool and should not depend on
compiler features, let alone fix its bugs, and therefore I think you should
more clearly spell out in your commit message that the code segment in
question is a workaround for an outright GCC bug, and just a kind of
"necessary evil" in given situation.

Let us also wait for David possibly coming up with the final verdict wrt
this issue.

Also, this patch should be split into two or three (infinity conversions
have nothing to do with xscvdpspn) - a patch deals with only one logical
unit.

Yours,
Aleksandar

> Sincerely,
> Aleksandar
>
> > Signed-off-by: Paul A. Clarke <pc@us.ibm.com>
> > ---
> >  target/ppc/fpu_helper.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> > index 5611cf0..82b5425 100644
> > --- a/target/ppc/fpu_helper.c
> > +++ b/target/ppc/fpu_helper.c
> > @@ -62,13 +62,14 @@ uint64_t helper_todouble(uint32_t arg)
> >          ret  = (uint64_t)extract32(arg, 30, 2) << 62;
> >          ret |= ((extract32(arg, 30, 1) ^ 1) * (uint64_t)7) << 59;
> >          ret |= (uint64_t)extract32(arg, 0, 30) << 29;
> > +        ret |= (0x7ffULL * (extract32(arg, 23, 8) == 0xff)) << 52;
> >      } else {
> >          /* Zero or Denormalized operand.  */
> >          ret = (uint64_t)extract32(arg, 31, 1) << 63;
> >          if (unlikely(abs_arg != 0)) {
> >              /* Denormalized operand.  */
> >              int shift = clz32(abs_arg) - 9;
> > -            int exp = -126 - shift + 1023;
> > +            int exp = -127 - shift + 1023;
> >              ret |= (uint64_t)exp << 52;
> >              ret |= abs_arg << (shift + 29);
> >          }
> > @@ -2871,10 +2872,14 @@ void helper_xscvqpdp(CPUPPCState *env, uint32_t
opcode,
> >
> >  uint64_t helper_xscvdpspn(CPUPPCState *env, uint64_t xb)
> >  {
> > +    uint64_t result;
> > +
> >      float_status tstat = env->fp_status;
> >      set_float_exception_flags(0, &tstat);
> >
> > -    return (uint64_t)float64_to_float32(xb, &tstat) << 32;
> > +    result = (uint64_t)float64_to_float32(xb, &tstat);
> > +    /* hardware replicates result to both words of the doubleword
result.  */
> > +    return (result << 32) | result;
> >  }
> >
> >  uint64_t helper_xscvspdpn(CPUPPCState *env, uint64_t xb)
> > --
> > 1.8.3.1
> >
> >

  reply	other threads:[~2019-08-17  7:55 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16 19:27 [Qemu-devel] [PATCH] ppc: Three floating point fixes Paul A. Clarke
2019-08-16 22:59 ` Aleksandar Markovic
2019-08-17  7:53   ` Aleksandar Markovic [this message]
2019-08-18  8:10   ` Richard Henderson
2019-08-18 20:59     ` Aleksandar Markovic
2019-08-19  6:28       ` David Gibson
2019-08-19  6:44         ` Aleksandar Markovic
2019-08-19 17:13           ` [Qemu-devel] [Qemu-ppc] " Paul Clarke
2019-08-20  7:31             ` David Gibson
2019-08-20 10:35               ` Peter Maydell
2019-08-22  3:02                 ` David Gibson
2019-08-18  8:00 ` [Qemu-devel] " Richard Henderson
2019-08-19  6:30 ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL1e-=hAcMMQMP4ecW_AW_3UM1yCjRE+P7QN3ytn2JaZeTFxUg@mail.gmail.com' \
    --to=aleksandar.m.mail@gmail.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=pc@us.ibm.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).