[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-bugs-dist
Subject:    [valgrind] [Bug 324894] Phase 3 support for IBM Power ISA 2.07
From:       Maynard Johnson <maynardj () us ! ibm ! com>
Date:       2013-09-30 18:26:52
Message-ID: bug-324894-17878-Tmra6egmtR () http ! bugs ! kde ! org/
[Download RAW message or body]

https://bugs.kde.org/show_bug.cgi?id=324894

--- Comment #8 from Maynard Johnson <maynardj@us.ibm.com> ---
(In reply to comment #7)
> These look fine to me.  Only comment is a sanity check about the
> saturating narrowing instructions:
Thanks for the review.
> 
> +   case 0x4CE: // vpkudus (Pack Signed Double Word Unsigned Saturate)
> +               binop(Iop_QNarrowBin64Uto32Ux4, mkexpr(vA), mkexpr(vB)) );
> 
> +   case 0x54E: { // vpksdus (Pack Signed Double Word Unsigned Saturate)
> +      putVReg( vD_addr, binop(Iop_QNarrowBin64Uto32Ux4,
> 
> +   case 0x5CE: // vpksdss (Pack Signed double word Signed Saturate)
> +               binop(Iop_QNarrowBin64Sto32Sx4, mkexpr(vA), mkexpr(vB)) );
> 
> The last one is OK, but the first two .. I was wondering if perhaps
> they should be using the S-to-U narrowing IROps, instead of the U-to-U
> ones as you have here.  I ask because of the description "Signed
> Double Word Unsigned Saturate"
> 
> In fact, is the text for the first one correct?  Should it be "Pack
> Unsigned Double Word Unsigned Saturate" ?  Then the UtoU op is
> correct; 
Yes, you're right, it should be "Pack Unsigned Double Word Unsigned Saturate".
> but the second one still doesn't look right.
Actually, it is correct . . . it's just a bit convoluted.  The comment for
vpksdus says the following:

      // This insn does a doubled signed->double unsigned saturating conversion
      // Conversion done here, then uses unsigned->unsigned vpk insn:
      //  => UnsignedSaturatingNarrow( x & ~ (x >>s 31) )

This technique (of doing a signed-to-unsigned conversion first, and then
unsigned-to-unsigned vector pack) was also used in the vpkswus instruction. 
Oops --  I see the DIP() has a copy/paste error with "vpkswus" instead of
vpksdus.  I'll write a small patch to the code patch that fixes this DIP and
the aforementioned incorrect text description for vpkudus.  I'll also add a
comment to the vpksdus to make it clear that we're using the same technique as
employed by vpkswus.

While I was looking at this, I saw that the testcase function for testing these
three vector pack saturate instructions prints the input arguments in the wrong
order. I'll attach a new testcase patch to fix that.
> 
[snip]

-- 
You are receiving this mail because:
You are watching all bug changes.
[prev in list] [next in list] [prev in thread] [next in thread]