RFR (XS) JDK-6961433: Revisit need to disable Windows C++ compiler optimisation of sharedRuntimeTrig.cpp

[Lois Foltan]

On 5/29/2014 9:37 PM, Vladimir Kozlov wrote:
> Hi Lois,
>
> What about sharedRuntimeTrans.cpp which also have switch off pragma?

Hi Vladimir,

Thank you for the review.  Yes, I am aware that sharedRuntimeTrans.cpp 
has a switch off pragma, but for certain reasons the bug, JDK-6961433 
was focused in on sharedRuntimeTrig.cpp.  After successfully testing the 
removal within sharedRuntimeTrig, I think it is then reasonable for us 
to look at the possibility of removing the pragma within 
sharedRuntimeTrans.cpp as well.

>
> Changes look correct but I am a little nervous. We were burned very 
> hard recently when we implemented exp/pow using x87 instructions in C2.
>
> Also we switch off optimization in makefiles for these files (I don't 
> know why):
>
> # The copied fdlibm routines in sharedRuntimeTrig.o must not be optimized
> OPT_CFLAGS/sharedRuntimeTrig.o = $(OPT_CFLAGS/NOOPT)
> # The copied fdlibm routines in sharedRuntimeTrans.o must not be 
> optimized
> OPT_CFLAGS/sharedRuntimeTrans.o = $(OPT_CFLAGS/NOOPT)

Good point, I did see this.  We switch off optimization in the makefiles 
for what seems to be the g++ compilers on linux & mac.  The Solaris and 
Windows makefiles do not explicity turn off optimization for these two 
files.  But of course on Windows the switch off pragma is present in the 
files.

>
> And I don't see performance results. Can you write tests similar to 
> next to measure performance and, most importantly, that sum result is 
> the same?

Makes sense, let me try to do something reasonable to measure the 
performance and when I have completed I will resubmit the RFR.

Thanks again,
Lois

>
> Thanks,
> Vladimir
>
> expintr$ cat ExpTime.java
> import java.lang.Math;
> import java.util.Random;
>
> public class ExpTime {
>
> static double[] buffer;
>
> public static void main(String[] args) {
>   buffer = new double[1000];
>   Random rand = new Random(435437646);
>   for(int i=0; i<1000; i++) {
>      buffer[i] = rand.nextDouble()*10;
>   }
>
>   double sum = 0.0;
>   for (int j = 0; j < 10; j++) {
>     sum += test(10000);
>   }
>   System.out.println("Warmup done...");
>   long start = System.currentTimeMillis();
>   sum += test(300000*5000);
>   long elapsedTimeMillis = System.currentTimeMillis()-start;
>   System.out.println("Iterations Per Micro Second:" + (300 * 
> 5000)/elapsedTimeMillis+" ipus");
>   System.out.println("Sum:" + sum);
> }
>
> private static double test(int len) {
>    double sum = 0.0;
>    for (int k = 0; k < len; k++) {
>       sum +=  Math.exp(buffer[(k)%1000]);
>    }
>    return sum;
> }
> }
>
> On 5/29/14 11:25 AM, Lois Foltan wrote:
>> Hello,
>>
>> Please review the following fix:
>>
>> Webrev:
>>      http://cr.openjdk.java.net/~lfoltan/bug_jdk6961433/
>>
>> Bug: Revisit need to disable Windows C++ compiler optimisation of
>> sharedRuntimeTrig.cpp
>>      https://bugs.openjdk.java.net/browse/JDK-6961433
>>
>> Summary of fix:
>> Remove WIN32 specific pragma optimize "off" within sharedRuntimeTrig.cpp
>> which resulted in the file being compiled with no optimizations.  The
>> SAFEBUF macro definition is also being removed.  It was a workaround
>> caused by the pragma being in effect. The problem report indicates that
>> this pragma was added in the VS2003/VS2005 time frame.  Where or how the
>> C++ compiler optimization manifested itself could not be located.  Thank
>> you to Dan Daugherty for completing a historical search to try to track
>> this down.  His search is posted in the problem report.  Also, thank you
>> to Vladimir Koslov for proposing ideas on how this should be tested.
>> Since VS2010 & higher should not have this optimization issue, the
>> pragma is being removed early in JDK 9 so it can benefit from continual
>> full testing.
>>
>> Tests:
>> JPRT build & test
>> vm.quick.testlist via Adhoc Aurora testing on Windows 32 & 64 bit - runs
>> with/without -Xint
>> JDK java/lang & java/util - runs with/without -Xint
>> Hotspot JTREG - runs with/without -Xint
>> Built full JDK 9 fastdebug and production images with this Hotspot
>> change, resulting image used for testing.