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

List:       openjdk-hotspot-compiler-dev
Subject:    Re: [9] RFR(S): 8160425: Vectorization with signalling NaN returns wrong result
From:       Tobias Hartmann <tobias.hartmann () oracle ! com>
Date:       2016-06-28 16:05:31
Message-ID: 5772A04B.5090208 () oracle ! com
[Download RAW message or body]

Thanks, Vladimir!

Best regards,
Tobias

On 28.06.2016 18:04, Vladimir Kozlov wrote:
> Looks good.
> 
> thanks,
> Vladimir
> 
> On 6/28/16 6:29 AM, Tobias Hartmann wrote:
> > Hi Vladimir,
> > 
> > thanks for the review!
> > 
> > On 28.06.2016 15:20, Vladimir Ivanov wrote:
> > > test/compiler/vectorization/TestNaNVector.java:
> > > 
> > > 56     public void vectorizeNaNDP() {
> > > ...
> > > 59         // should look like this '0xffa0ffa0ffa0ffa0' and is read from the \
> > >                 constant
> > > ...
> > > 63         // as '0xffa0ffa0ffa0ffa0' which leads to an incorrect result.
> > > 
> > > The comment in the test is misleading.
> > 
> > Right, I first used a different constant and forgot to update the comment. The \
> > description in the RFR is correct. 
> > Also used the wrong bug id for the first webrev. Here's the new one:
> > http://cr.openjdk.java.net/~thartmann/8160425/webrev.01/
> > 
> > Thanks,
> > Tobias
> > 
> > > Otherwise, looks good.
> > > Best regards,
> > > Vladimir Ivanov
> > > 
> > > On 6/28/16 2:47 PM, Tobias Hartmann wrote:
> > > > Hi,
> > > > 
> > > > please review the following patch:
> > > > 
> > > > https://bugs.openjdk.java.net/browse/JDK-8160425
> > > > http://cr.openjdk.java.net/~thartmann/8160425/webrev.00/
> > > > 
> > > > We vectorize the following loop:
> > > > 
> > > > for (int i = 0; i < 1024; ++i) {
> > > > array[i] = (char)0xfff7;
> > > > }
> > > > 
> > > > The array store is replaced by a 64-bit vector store to four subsequent array \
> > > > elements. The vector should look like this '0xfff7fff7fff7fff7' and is read \
> > > > from the constant table. However, in floating point arithmetic, this is a \
> > > > signalling NaN which is converted to a quiet NaN when processed by the x87 \
> > > > FPU. After the signalling bit is set, the vector ends up in the constant \
> > > > table as '0xfffffff7fff7fff7' which leads to an incorrect result: 
> > > > [Constants]
> > > > 0xee376ec0 (offset:    0): 0xfff7fff7   0xfffffff7fff7fff7
> > > > 0xee376ec4 (offset:    4): 0xfffffff7
> > > > 
> > > > The problem is that the constant vector is passed around as a double in the C \
> > > > code. On x86 32-bit, floating point values are returned via the FPU stack and \
> > > > since the value is a signalling NaN, it's converted to a quiet NaN by the FPU \
> > > > instructions. In this case, 'replicate8_imm' returns a double value which is \
> > > > passed to the caller on the FPU stack. Because the value is a signalling NaN, \
> > > > the 'fldl' instruction sets the most-significant fraction bit to 1 (see \
> > > > Chapter 4.8.3.4 of the Intel manual [1] or [2] for a similar problem). 
> > > > This code was introduced by JDK-7119644 [3]. I think we should not use \
> > > > doubles/floats in the C code but ints/longs because those constants should \
> > > > not be treated as floating point values. For consistency, I also changed this \
> > > > on SPARC although I'm not aware of any problems there. With the fix, the \
> > > > constant table contains the correct value: 
> > > > [Constants]
> > > > 0xee4154c0 (offset:    0): 0xfff7fff7   0xfff7fff7fff7fff7
> > > > 0xee4154c4 (offset:    4): 0xfff7fff7
> > > > 
> > > > Tested with failing testcase, JPRT and RBT (running).
> > > > 
> > > > Thanks,
> > > > Tobias
> > > > 
> > > > [1] http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-manual-325462.pdf
> > > >  [2] http://stackoverflow.com/questions/22816095/signalling-nan-was-corrupted-when-returning-from-x86-function-flds-fstps-of-x87
> > > >  [3] https://bugs.openjdk.java.net/browse/JDK-7119644
> > > > 


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