Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[joe darcy]

Hello,

On 9/12/2018 1:16 AM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?

In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?




To condense a potentially long discussion, while the IEEE 754 standard 
has long specified particular results for arithmetic operations (+, -, 
*, /, etc.) on particular floating-point values, languages and their 
compilers often do not provide a reliable mapping of language constructs 
to IEEE 754 operations.


The Java language and JVM are distinctive in this sense because a 
reliable mapping of language-level operation to particular IEEE 754 
operation is mandated by the JLS. (I will leave aside a complicated but 
largely irrelevant discussion of non-strictfp floating-point.)


The C language standards I've looked at do not provide as reliably a 
mapping of floating-point operations as the JLS does. In particular, the 
C standards generally allow a fused multiply add to be used replace a 
pair of add and multiply instructions in an expression like (a * b + c). 
The -ffp-contract=off gcc compiler setting disables this and related 
transformations. (The Sun Studio compilers provide detailed 
configuration options for the sets of floating-point transformations 
that are allowed.)


The specification for StrictMath requires the fdlibm algorithms and the 
fdlibm algorithms rely on the semantics of the floating-point operations 
as written in the source and also rely on some way of doing a bit-wise 
conversion between an integral type and double. The latter is 
accomplished by interpreting the 64-bits of a double as comprising a 
two-element array of 32-bit ints. These idioms often don't work under 
the default C compiler options, leading to the long-standing need to 
have a separate set of compiler options for FDLIBM. A safe, if slow, set 
of options is to fully disable optimization for FDLIBM. That is not 
necessary if sufficient control over the floating-point and aliasing 
semantics is possible via the C compiler options.


In the fullness of time, when (if?) I finish porting the FDLIBM code to 
Java, these sorts of concerns will no longer apply due to the more 
reliably mapping of source expressions in Java to IEEE 754 
floating-point operations.


HTH,

-Joe

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

Correction ...

On 13/09/2018 8:31 AM, David Holmes wrote:

On 12/09/2018 6:16 PM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?


In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63 



We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?


Yes that makes sense - thanks. I didn't quite glean that from the comment:

   42 # If FDLIBM_CFLAGS is non-empty we know that we can optimize
   43 # fdlibm by adding those extra C flags. Currently GCC,


I think this should say "when adding" not "by adding".

Thanks,
David


   44 # and clang only.
   45 ifneq ($(FDLIBM_CFLAGS), )
   46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

But now I can read it and understand.

Thanks,
David


Thanks,
Severin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

On 12/09/2018 6:16 PM, Severin Gehwolf wrote:

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:

But I don't understand why the optimization setting is being tied to the
availability of the -ffp-contract flag?


In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?


Yes that makes sense - thanks. I didn't quite glean that from the comment:

  42 # If FDLIBM_CFLAGS is non-empty we know that we can optimize
  43 # fdlibm by adding those extra C flags. Currently GCC,
  44 # and clang only.
  45 ifneq ($(FDLIBM_CFLAGS), )
  46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

But now I can read it and understand.

Thanks,
David


Thanks,
Severin

Re: [llvm-dev] OpenJDK8 failed to work after compiled by LLVM 8 for X86

[Dimitry Andric]

Hi Leslie,

The problem really lies in the OpenJDK code, as it is attempting to
write to a const object.  If this seems to work with certain compiler(s)
and optimization settings, it is just luck. :-)

Here is a reduced example, which shows the issue:

==
#include 

class NativeCallStack {
public:
  static const NativeCallStack EMPTY_STACK;

private:
  enum { DEPTH = 4 };
  void* stack[DEPTH];

public:
  NativeCallStack() {
for (int i = 0; i < DEPTH; ++i) {
  stack[i] = nullptr;
}
  }
};

const NativeCallStack NativeCallStack::EMPTY_STACK;

int main(void)
{
  // The following should segfault, if EMPTY_STACK was placed into .rodata.
  ::new ((void*)::EMPTY_STACK) NativeCallStack();
  return 0;
}
==

clang 7.0.0 rc2 produces this assembly:

.globl  main# -- Begin function main
.p2align4, 0x90
.type   main,@function
main:   # @main
.cfi_startproc
# %bb.0:# %entry
pushq   %rbp
.cfi_def_cfa_offset 16
.cfi_offset %rbp, -16
movq%rsp, %rbp
.cfi_def_cfa_register %rbp
xorps   %xmm0, %xmm0
movups  %xmm0, NativeCallStack::EMPTY_STACK+16(%rip)
movups  %xmm0, NativeCallStack::EMPTY_STACK(%rip)
xorl%eax, %eax
popq%rbp
.cfi_def_cfa %rsp, 8
retq
.Lfunc_end0:
.size   main, .Lfunc_end0-main
.cfi_endproc
# -- End function
.type   NativeCallStack::EMPTY_STACK,@object # 
@NativeCallStack::EMPTY_STACK
.section.rodata,"a",@progbits
.globl  NativeCallStack::EMPTY_STACK
.p2align3
NativeCallStack::EMPTY_STACK:
.zero   32
.size   NativeCallStack::EMPTY_STACK, 32

The whole constructor has been optimized out, and EMPTY_STACK has been
replaced by .zero 32, placed in the .rodata segment, which is a
perfectly valid transformation.

Then, in main(), an attempt is made to write to that read-only segment,
which will usually segfault, depending on how strictly the underlying
operating system enforces these protections.

In contrast, gcc 8.2.0 produces:

.globl  main
.type   main, @function
main:
.LFB74:
.cfi_startproc
movq$0, NativeCallStack::EMPTY_STACK(%rip)
xorl%eax, %eax
movq$0, NativeCallStack::EMPTY_STACK+8(%rip)
movq$0, NativeCallStack::EMPTY_STACK+16(%rip)
movq$0, NativeCallStack::EMPTY_STACK+24(%rip)
ret
.cfi_endproc
.LFE74:
.size   main, .-main
.p2align 4,,15
.type   _GLOBAL__sub_I__ZN15NativeCallStack11EMPTY_STACKE, @function
_GLOBAL__sub_I__ZN15NativeCallStack11EMPTY_STACKE:
.LFB76:
.cfi_startproc
movq$0, NativeCallStack::EMPTY_STACK(%rip)
movq$0, NativeCallStack::EMPTY_STACK+8(%rip)
movq$0, NativeCallStack::EMPTY_STACK+16(%rip)
movq$0, NativeCallStack::EMPTY_STACK+24(%rip)
ret
.cfi_endproc
.LFE76:
.size   _GLOBAL__sub_I__ZN15NativeCallStack11EMPTY_STACKE, 
.-_GLOBAL__sub_I__ZN15NativeCallStack11EMPTY_STACKE
.section.ctors,"aw",@progbits
.align 8
.quad   _GLOBAL__sub_I__ZN15NativeCallStack11EMPTY_STACKE
.globl  NativeCallStack::EMPTY_STACK
.bss
.align 32
.type   NativeCallStack::EMPTY_STACK, @object
.size   NativeCallStack::EMPTY_STACK, 32
NativeCallStack::EMPTY_STACK:
.zero   32

Here you can see that gcc decided to not place EMPTY_STACK in .rodata,
but in .bss, which is read/write.  It adds the global constructor to
the .ctors section, so the dynamic linker is made responsible for
calling it.

Note that now the EMPTY_STACK object is unnecessarily initialized twice,
once by the global constructor, and once in main.

-Dimitry

> On 11 Sep 2018, at 03:38, Leslie Zhai  wrote:
> 
> Hi Dimitry,
> 
> Thanks for your kind response!
> 
> Thanks for the commit message of Jung's patch,  I found that the bug had been 
> fixed in OpenJDK 12 by Zhengyu 
> https://bugs.openjdk.java.net/browse/JDK-8205965  But only backported to 11.  
> So Jung could backport it for OpenJDK 8, thanks a lot!
> 
> But I argue that the root cause might be in the compiler side, why 
> clang-3.9.1, gcc-6.4.1 couldn't reproduce the bug?  And it might be work for 
> clang-4.0 https://bugs.openjdk.java.net/browse/JDK-8208494  I will test LLVM 
> 5 to check out whether or not reproducible.
> 
> 
> 在 2018年09月11日 00:59, Dimitry Andric 写道:
>> Hi Leslie,
>> 
>> This is likely the same problem as was reported in 
>> https://bugs.freebsd.org/225054#c8, and fixed by the following patch:
>> 
>> 

Re: [llvm-dev] OpenJDK8 failed to work after compiled by LLVM 8 for X86

[Dimitry Andric]

Hi Leslie,

This is likely the same problem as was reported in 
https://bugs.freebsd.org/225054#c8, and fixed by the following patch:

https://svnweb.freebsd.org/ports/head/java/openjdk8/files/patch-hotspot_src_share_vm_services_memTracker.cpp?view=markup=459368

Can you please try that out, and see if it fixes it for you too?

-Dimitry

> On 10 Sep 2018, at 18:20, Leslie Zhai via llvm-dev  
> wrote:
> 
> Hi all,
> 
> OpenJDK8 jdk8u-dev[1] is just able to work after compiled with LLVM 3.9.1 for 
> X86:
> 
> $ ./build/linux-x86_64-normal-server-slowdebug/images/j2sdk-image/bin/java 
> -version
> openjdk version "1.8.0-internal-debug"
> OpenJDK Runtime Environment (build 
> 1.8.0-internal-debug-xiangzhai_2018_09_09_21_08-b00)
> OpenJDK 64-Bit Server VM (build 25.71-b00-debug, mixed mode)
> 
> $ strings 
> ./build/linux-x86_64-normal-server-slowdebug/images/j2sdk-image/bin/java | 
> grep clang
> clang version 3.9.1 (tags/RELEASE_391/final)
> 
> But it failed to work after compiled with LLVM 8 for X86:
> 
> $ ./build/linux-x86_64-normal-server-fastdebug/images/j2sdk-image/bin/java 
> -version
> Segmentation fault
> 
> $ gdb --ex run --args 
> ./build/linux-x86_64-normal-server-fastdebug/images/j2sdk-image/bin/java 
> -version
> GNU gdb (GDB) Fedora 7.12.1-48.fc25
> ...
> Starting program: 
> /home/xiangzhai/project/jdk8u-llvm/build/linux-x86_64-normal-server-fastdebug/images/j2sdk-image/bin/java
>  -version
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib64/libthread_db.so.1".
> 
> Program received signal SIGSEGV, Segmentation fault.
> [ Legend: Modified register | Code | Heap | Stack | String ]
> ───[
>  registers ]
> $rax   : 0x0
> $rbx   : 0x0
> $rcx   : 0x77315eb0  →  0x76ae30a0  → 
>  xor edx, edx
> $rdx   : 0x0
> $rsp   : 0x7fff9328  →  0x76a76822  → 
>  mov edi, ebx
> $rbp   : 0x7fff93c0  →  0x7fff94f0  → 0x7fff9510  →  
> 0x0002
> $rsi   : 0x0
> $rdi   : 0x77315e70  →  0x77315eb0  → 0x76ae30a0  → 
>  xor edx, edx
> $rip   : 0x76ae2f5b  →  mov 
> QWORD PTR [rdi], rcx
> $r8: 0x1
> $r9: 0xf
> $r10   : 0x64
> $r11   : 0x0
> $r12   : 0x7fffdc88  →  0x7fffe04c  → 
> "/home/xiangzhai/project/jdk8u-llvm/build/linux-x86[...]"
> $r13   : 0x7fffdca0  →  0x7fffe0bf  → "XDG_VTNR=1"
> $r14   : 0x7fff9330  →  "NMT_LEVEL_10535"
> $r15   : 0x6
> $eflags: [carry parity adjust zero sign trap INTERRUPT direction overflow 
> RESUME virtualx86 identification]
> $ss: 0x002b  $ds: 0x  $cs: 0x0033  $es: 0x  $gs: 0x $fs: 0x
> ───[
>  stack ]
> 0x7fff9328│+0x00: 0x76a76822  → 
>  mov edi, ebx← $rsp
> 0x7fff9330│+0x08: "NMT_LEVEL_10535"  ← $r14
> 0x7fff9338│+0x10: 0x0035333530315f4c ("L_10535"?)
> 0x7fff9340│+0x18: 0x0001
> 0x7fff9348│+0x20: 0x00602190  → 0x75eb7000  →  
> 0x00010102464c457f
> 0x7fff9350│+0x28: 0x0004
> 0x7fff9358│+0x30: 0x00066474e551
> 0x7fff9360│+0x38: 0x
> [
>  code:i386:x86-64 ]
>0x76ae2f4b  addeax, 
> 0x1f0f00
>0x76ae2f50  movrcx, QWORD 
> PTR [rip+0x842781]# 0x773256d8
>0x76ae2f57  addrcx, 0x10
>  → 0x76ae2f5b  movQWORD PTR 
> [rdi], rcx
>0x76ae2f5e  movDWORD PTR 
> [rdi+0x28], 0x0
>0x76ae2f65  addrdi, 0x8
>0x76ae2f69  test   edx, edx
>0x76ae2f6b  je 
> 0x76ae2f7b 
>0x76ae2f6d  moveax, esi
> ─[ 
> source:/home/xiangzhai/project/jdk8u-llvm/hotspot/src/share/vm/utilities/nativeCallStack.cpp+33
>  ]
>  28  #include "utilities/nativeCallStack.hpp"
>  29
>  30  const NativeCallStack NativeCallStack::EMPTY_STACK(0, false);
>  31
>  32  NativeCallStack::NativeCallStack(int toSkip, bool fillStack) :
>  →   33_hash_value(0) {
>  34
>  35  #if !PLATFORM_NATIVE_STACK_WALKING_SUPPORTED
>  36fillStack = false;
>  37  #endif
>  38
> ─[
>  threads ]
> [#0] Id 1, Name: "java", stopped, reason: SIGSEGV
> ───[
>  trace ]
> [#0] 0x76ae2f5b → Name: 
> NativeCallStack::NativeCallStack(this=0x77315e70 
> , toSkip=0x0, fillStack=0x0)
> [#1] 0x76a76822 → Name: MemTracker::init_tracking_level()
> [#2] 0x762eff49 → Name: MemTracker::tracking_level()
> [#3] 0x762eff49 → Name: ResourceObj::operator new(size=0x20, 
> 

Re: 8210425: [x86] sharedRuntimeTrig/sharedRuntimeTrans compiled without optimization

[Schmidt, Lutz]

I totally agree with Andrew's statement. FP calculations should be evaluated as 
the programmer wrote them down. All fiddling around with sequence or rounding 
is evil. You lose reproducibility of results. 
Regards,
Lutz

On 08.09.18, 11:26, "hotspot-dev on behalf of Andrew Haley" 
 wrote:

On 09/06/2018 03:32 PM, Severin Gehwolf wrote:
> Right. I should note that ppc64, s390x and aarch64 ports don't have
> this optimization turned off as those overrides are in a x86 specific
> block. It appears it hasn't caused issues for these ports so far.

That's just dumb luck. We really should turn off the merging of FP
operations on all platforms.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: [12]: RFR: 8209167: Use CLDR's time zone mappings for Windows

[Erik Joelsson]

Looks good.

/Erik


On 2018-09-12 10:33, naoto.s...@oracle.com wrote:

Hi Magnus, Erik,

Thank you for your comments. I updated the webrev according to your 
suggestions:


http://cr.openjdk.java.net/~naoto/8209167/webrev.02/

As to the duplicated "common" in the dependency, yes that's a separate 
issue. Since it's obvious, I included the fix with this changeset (it 
was actually described as a comment in the jira issue).


Naoto

On 9/12/18 9:08 AM, Erik Joelsson wrote:

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we 
test more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the 
easiest workaround for two files generated from the same rule. It 
sort of works (for clean builds and if the input is chagned), but 
won't handle all cases where one file is removed externally and the 
other is not. I suggested this construct to Naoto. Perhaps a comment 
explaining this would be good.
The removal of the duplicate "common", that seems like a separate 
bug fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: [12]: RFR: 8209167: Use CLDR's time zone mappings for Windows

[naoto . sato]

Hi Magnus, Erik,

Thank you for your comments. I updated the webrev according to your 
suggestions:


http://cr.openjdk.java.net/~naoto/8209167/webrev.02/

As to the duplicated "common" in the dependency, yes that's a separate 
issue. Since it's obvious, I included the fix with this changeset (it 
was actually described as a comment in the jira issue).


Naoto

On 9/12/18 9:08 AM, Erik Joelsson wrote:

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the easiest 
workaround for two files generated from the same rule. It sort of works 
(for clean builds and if the input is chagned), but won't handle all 
cases where one file is removed externally and the other is not. I 
suggested this construct to Naoto. Perhaps a comment explaining this 
would be good.
The removal of the duplicate "common", that seems like a separate bug 
fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: Why is vmStructs.cpp compiled with -O0?

[Erik Joelsson]

Hello,

I very much doubt it was included with the new build system. We were 
extremely careful to use the exact same flags then, and did binary 
comparisons of all object files to verify equal builds.


Tracing back, it was caused (probably unintentionally) by this change:

http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/e796d52ca85b

In the old build, setting OPT_CFLAGS/ overrides the common 
OPT_CFLAGS. In the new build, we have a more general way of adding flags 
for specific files that does not directly override any other flags. To 
get the same behavior for vmStructs.cpp in the new build as in the old 
at the time of the switch, we had to add -O0 explicitly in the new build.


/Erik


On 2018-09-12 09:13, Severin Gehwolf wrote:

Hi,

Does anybody know why vmStructs.cpp gets an override in
JvmOverrideFiles.gmk:

$ grep -C3 -n vmStructs.cpp make/hotspot/lib/JvmOverrideFiles.gmk
30-# status for individual files on specific platforms.
31-
32-ifeq ($(TOOLCHAIN_TYPE), gcc)
33:  BUILD_LIBJVM_vmStructs.cpp_CXXFLAGS := -fno-var-tracking-assignments -O0
34-  BUILD_LIBJVM_jvmciCompilerToVM.cpp_CXXFLAGS := 
-fno-var-tracking-assignments
35-  BUILD_LIBJVM_jvmciCompilerToVMInit.cpp_CXXFLAGS := 
-fno-var-tracking-assignments
36-  BUILD_LIBJVM_assembler_x86.cpp_CXXFLAGS := -Wno-maybe-uninitialized

It seems to have been introduced with the new build system. JDK 8
doesn't seem to have it. Thoughts?

Thanks,
Severin

Re: 8210425: [x86] sharedRuntimeTrig/sharedRuntimeTrans compiled without optimization

[Erik Joelsson]

Hello Severin,

In configure, we now set FDLIBM_CFLAGS based on toolchain type and 
capabilities. In JvmOverrideFiles.gmk, the use of it is still 
conditional on Linux. I don't think it should be. We already have the 
conditionals we need.


/Erik


On 2018-09-12 05:44, Severin Gehwolf wrote:

Hi,

Updated webrev since fdlibm build change seems to have settled:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210425/webrev.02/

Optimization is being done at level -O2 (CXX_O_FLAG_NORM) with
-ffp-contract=off when the toolchain (gcc/clang at this point)
supports it. Otherwise no optimization will be done. Those overrides
are no longer in an x86 specific block. Thus, ppc64, aarch64, s390x
will get correct settings too.

How does this look?

Thanks,
Severin

On Wed, 2018-09-12 at 07:16 +, Schmidt, Lutz wrote:

I totally agree with Andrew's statement. FP calculations should be
evaluated as the programmer wrote them down. All fiddling around with
sequence or rounding is evil. You lose reproducibility of results.
Regards,
Lutz

On 08.09.18, 11:26, "hotspot-dev on behalf of Andrew Haley" 
 wrote:

 On 09/06/2018 03:32 PM, Severin Gehwolf wrote:
 > Right. I should note that ppc64, s390x and aarch64 ports don't have
 > this optimization turned off as those overrides are in a x86 specific
 > block. It appears it hasn't caused issues for these ports so far.
 
 That's just dumb luck. We really should turn off the merging of FP

 operations on all platforms.
 
 --

 Andrew Haley
 Java Platform Lead Engineer
 Red Hat UK Ltd. 
 EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
 

Re: Linux + Clang + execstack

[Martin Buchholz]

On Wed, Sep 12, 2018 at 4:01 AM, Magnus Ihse Bursie
 wrote:
> On 2018-09-05 20:59, Martin Buchholz wrote:
>
> So ... Magnus, are you happy with the current state of the proposed patch?
>
> I'm sorry Martin, but I can't figure out what the current state is. I tried
> backtracking the discussion but failed. :( Can you please repost the
> currently proposed patch?

http://cr.openjdk.java.net/~martin/webrevs/jdk/noexecstack/noexecstack.patch

> On Tue, Sep 4, 2018 at 11:50 PM, Magnus Ihse Bursie
>  wrote:
>>
>>
>> For the gcc toolchain this can not be the case:
>> # Minimum supported linker versions, empty means unspecified
>> TOOLCHAIN_MINIMUM_LD_VERSION_gcc="2.18"
>>
>> We make sure we have an ld that supports the basic flags we assume.
>
>
> feature tests are better than version tests.
>
> I've heard that argument many times, and I've never agreed with it. In some
> cases, feature tests work. They typically work if someone has designed a
> clear API and included a feature test in it. A lot of additional POSIX
> functionality works that way. This means that you can rest assure that if
> the feature is present, then you know what you are going to get.
>
> In most of the rest of the world, functionality does not raise to that
> golden standard. Gcc adds a flag in one version, but it's buggy. A later
> version fixes the bugs. A later version still changes the behavior of the
> flag. Functionality that we depend on works or does not works depending on
> the intersection of things like our code, compiler version, operating
> system, and so on.
>
> In my experience, it's a rare thing for a feature test to actually work.
> Version tests, on the other hand, tests against a specific setup, that can
> be tested and proven to work. The downside of version tests is that they are
> often open-ended; we say that "anything above this version is supposed to
> work", even though we have not tested with gcc 8 or 9. The alternative is to
> say that "we've tested this between gcc 4.7 and 7.3 and will only support it
> for this version span", but that is in most cases more likely to break when
> gcc 8 comes along.

Specific version tests are in principle more accurate, but they
require a level of testing and maintenance that is unlikely to be seen
in the real world.  The received wisdom is that one should prefer
feature tests whenever possible and I agree with that as well, based
on decades of experience.

Sometimes you need something in between, e.g. replacing a
configure-time check with a run-time check.

Why is vmStructs.cpp compiled with -O0?

[Severin Gehwolf]

Hi,

Does anybody know why vmStructs.cpp gets an override in
JvmOverrideFiles.gmk:

$ grep -C3 -n vmStructs.cpp make/hotspot/lib/JvmOverrideFiles.gmk
30-# status for individual files on specific platforms.
31-
32-ifeq ($(TOOLCHAIN_TYPE), gcc)
33:  BUILD_LIBJVM_vmStructs.cpp_CXXFLAGS := -fno-var-tracking-assignments -O0
34-  BUILD_LIBJVM_jvmciCompilerToVM.cpp_CXXFLAGS := 
-fno-var-tracking-assignments
35-  BUILD_LIBJVM_jvmciCompilerToVMInit.cpp_CXXFLAGS := 
-fno-var-tracking-assignments
36-  BUILD_LIBJVM_assembler_x86.cpp_CXXFLAGS := -Wno-maybe-uninitialized

It seems to have been introduced with the new build system. JDK 8
doesn't seem to have it. Thoughts?

Thanks,
Severin

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Erik Joelsson]

On 2018-09-12 03:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


It's generated by the same rule as the other file. This is the easiest 
workaround for two files generated from the same rule. It sort of works 
(for clean builds and if the input is chagned), but won't handle all 
cases where one file is removed externally and the other is not. I 
suggested this construct to Naoto. Perhaps a comment explaining this 
would be good.
The removal of the duplicate "common", that seems like a separate bug 
fix?



It does indeed. Before this fix, the wildcards must have returned empty.

/Erik

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Gustavo Romero]

Hi Andrew,

On 09/12/2018 12:51 AM, Gustavo Romero wrote:


Maybe Andrew can enlight us.


I was not sure if 'enlight us' was the correct form here, so I did some search
and I found with horror today that 'enlighten us' can also be considered an
insult (!?).

That's really not the case here. So if it's really possible to interpret it in
a non-positive / non-cordial way I sincerely apologize for the wrong use.

I was really only asking for an advice. :)


Regards,
Gustavo

Re: [llvm-dev] OpenJDK8 failed to work after compiled by LLVM 8 for X86

[Zhengyu Gu]

Probably should also backport the followup RFE: 
https://bugs.openjdk.java.net/browse/JDK-8206183


Thanks,

-Zhengyu

On 09/11/2018 10:58 PM, David Holmes wrote:
Or to be a little less obscure, this is a known issue and you should 
look into backporting:


https://bugs.openjdk.java.net/browse/JDK-8205965

David

On 12/09/2018 5:03 AM, Martin Buchholz wrote:

https://openjdk.markmail.org/thread/rwfcd6df6vhzli5m

Re: 8210425: [x86] sharedRuntimeTrig/sharedRuntimeTrans compiled without optimization

[Severin Gehwolf]

Hi,

Updated webrev since fdlibm build change seems to have settled:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210425/webrev.02/

Optimization is being done at level -O2 (CXX_O_FLAG_NORM) with
-ffp-contract=off when the toolchain (gcc/clang at this point)
supports it. Otherwise no optimization will be done. Those overrides
are no longer in an x86 specific block. Thus, ppc64, aarch64, s390x
will get correct settings too.

How does this look?

Thanks,
Severin

On Wed, 2018-09-12 at 07:16 +, Schmidt, Lutz wrote:
> I totally agree with Andrew's statement. FP calculations should be
> evaluated as the programmer wrote them down. All fiddling around with
> sequence or rounding is evil. You lose reproducibility of results. 
> Regards,
> Lutz
> 
> On 08.09.18, 11:26, "hotspot-dev on behalf of Andrew Haley" 
>  wrote:
> 
> On 09/06/2018 03:32 PM, Severin Gehwolf wrote:
> > Right. I should note that ppc64, s390x and aarch64 ports don't have
> > this optimization turned off as those overrides are in a x86 specific
> > block. It appears it hasn't caused issues for these ports so far.
> 
> That's just dumb luck. We really should turn off the merging of FP
> operations on all platforms.
> 
> -- 
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. 
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
> 
> 

Re: RFF (12): JDK-8207954: Data for --release 11

[Magnus Ihse Bursie]

On 2018-09-12 13:49, Jan Lahoda wrote:

Hi,

I've updated the data patch to include the recently added 
java.net.http.HttpConnectTimeoutException (see JDK-8209187):

http://cr.openjdk.java.net/~jlahoda/8207954/webrev.data.01/

The code patch remains unchanged from the last round:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.01/

Any feedback on this?


No objections from the build perspective.

/Magnus



Thanks,
    Jan

On 25.7.2018 17:15, Jan Lahoda wrote:

Thanks Erik. I've updated the code patch here:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.01/

(the data patch remains the same)

Does this look better?

Thanks,
 Jan

On 23.7.2018 18:56, Erik Joelsson wrote:

Hello Jan,

The assignment of a non existent directory looks suspect. You should be
able assign to an empty value using ?=. On the other hand, what it 
looks
like you want to achieve is having both the open and closed value on 
the

command line. Then I would just change the ?= to +=.

/Erik


On 2018-07-23 04:44, Jan Lahoda wrote:

Hi,

To support --release 11 in JDK 12, historical data for JDK 11 need to
be added. As new attributes (NestHost and NestMembers) have been added
to the classfile, there also needs to be some tweaking of the tool
that creates the historical descriptions and the ct.sym sigfiles.

The proposed patch has two parts:
-code changes, which include:
--update to the build scripts and the tool, so that the tool can read
both the open and extra/closed description of the ct.sym content, and
merge those (there should not be any extra/closed for JDK 11, AFAIK,
so this is to avoid having to update the closed part).
--improving the description on how to add historical data for new
releases, using the new source launcher
--adding support for the new NestHost/NestMembers attributes
--adding a new test which strives to fail when a new attribute is
added and CreateSymbols is not updated
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.00/

-addition of the historical data:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.data.00/
(these may need to be re-generated when the final JDK 11 is released
in case there are any changes, but that should be very simple)

JBS: https://bugs.openjdk.java.net/browse/JDK-8207954

Any feedback is welcome,
 Jan

Re: RFF (12): JDK-8207954: Data for --release 11

[Jan Lahoda]

Hi,

I've updated the data patch to include the recently added 
java.net.http.HttpConnectTimeoutException (see JDK-8209187):

http://cr.openjdk.java.net/~jlahoda/8207954/webrev.data.01/

The code patch remains unchanged from the last round:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.01/

Any feedback on this?

Thanks,
Jan

On 25.7.2018 17:15, Jan Lahoda wrote:

Thanks Erik. I've updated the code patch here:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.01/

(the data patch remains the same)

Does this look better?

Thanks,
 Jan

On 23.7.2018 18:56, Erik Joelsson wrote:

Hello Jan,

The assignment of a non existent directory looks suspect. You should be
able assign to an empty value using ?=. On the other hand, what it looks
like you want to achieve is having both the open and closed value on the
command line. Then I would just change the ?= to +=.

/Erik


On 2018-07-23 04:44, Jan Lahoda wrote:

Hi,

To support --release 11 in JDK 12, historical data for JDK 11 need to
be added. As new attributes (NestHost and NestMembers) have been added
to the classfile, there also needs to be some tweaking of the tool
that creates the historical descriptions and the ct.sym sigfiles.

The proposed patch has two parts:
-code changes, which include:
--update to the build scripts and the tool, so that the tool can read
both the open and extra/closed description of the ct.sym content, and
merge those (there should not be any extra/closed for JDK 11, AFAIK,
so this is to avoid having to update the closed part).
--improving the description on how to add historical data for new
releases, using the new source launcher
--adding support for the new NestHost/NestMembers attributes
--adding a new test which strives to fail when a new attribute is
added and CreateSymbols is not updated
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.code.00/

-addition of the historical data:
http://cr.openjdk.java.net/~jlahoda/8207954/webrev.data.00/
(these may need to be re-generated when the final JDK 11 is released
in case there are any changes, but that should be very simple)

JBS: https://bugs.openjdk.java.net/browse/JDK-8207954

Any feedback is welcome,
 Jan

Re: RFR: 8210459: Add support for generating compile_commands.json

[Magnus Ihse Bursie]

On 2018-09-10 15:36, Robin Westberg wrote:

Hi all,

Please review the following change that adds support for generating compile_commands.json 
as a top-level make target. This is a popular format for describing how to compile object 
files for a project, and is defined in [1]. This file can be used directly by IDEs such 
as Visual Studio Code and CLion as well as "language servers" such as cquery 
[2] and rtags [3].

While it’s currently possible to generate this file as part of a full build 
(using tools such as Bear [4], or simply parsing .cmdline files), this change 
aims to make it possible to generate the compile commands data without actually 
compiling the object files. This means that it’s for example possible to start 
using an IDE fairly quickly on freshly cloned source, instead of having to wait 
for a full build to complete.

This was originally inspired by the work done in [5] by Mikael Gerdin and Erik 
Joelsson, but has been through a few revisions since then. Erik has also 
provided a lot of assistance with the current version, thanks Erik!

Issue: https://bugs.openjdk.java.net/browse/JDK-8210459
Webrev: http://cr.openjdk.java.net/~rwestberg/8210459/webrev.00/
That's a lot of changes, some quite intrusive. I'm glad you've had help 
from Erik to get this patch right, but I'd like to be able to examine it 
a bit more closely. I'll get back to you with a full review.


I'm glad you're doing this! I think this in general is good, I just want 
it do be done the right way. :)


/Magnus



Testing: tier1, builds-tier5

Best regards,
Robin

[1] https://clang.llvm.org/docs/JSONCompilationDatabase.html
[2] https://github.com/cquery-project/cquery
[3] https://github.com/Andersbakken/rtags
[4] https://github.com/rizsotto/Bear
[5] http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-March/026354.html

Re: Linux + Clang + execstack

[Magnus Ihse Bursie]

On 2018-09-05 20:59, Martin Buchholz wrote:

So ... Magnus, are you happy with the current state of the proposed patch?
I'm sorry Martin, but I can't figure out what the current state is. I 
tried backtracking the discussion but failed. :( Can you please repost 
the currently proposed patch?





On Tue, Sep 4, 2018 at 11:50 PM, Magnus Ihse Bursie 
mailto:magnus.ihse.bur...@oracle.com>> 
wrote:



For the gcc toolchain this can not be the case:
# Minimum supported linker versions, empty means unspecified
TOOLCHAIN_MINIMUM_LD_VERSION_gcc="2.18"

We make sure we have an ld that supports the basic flags we assume.


feature tests are better than version tests.
I've heard that argument many times, and I've never agreed with it. In 
some cases, feature tests work. They typically work if someone has 
designed a clear API and included a feature test in it. A lot of 
additional POSIX functionality works that way. This means that you can 
rest assure that if the feature is present, then you know what you are 
going to get.


In most of the rest of the world, functionality does not raise to that 
golden standard. Gcc adds a flag in one version, but it's buggy. A later 
version fixes the bugs. A later version still changes the behavior of 
the flag. Functionality that we depend on works or does not works 
depending on the intersection of things like our code, compiler version, 
operating system, and so on.


In my experience, it's a rare thing for a feature test to actually work. 
Version tests, on the other hand, tests against a specific setup, that 
can be tested and proven to work. The downside of version tests is that 
they are often open-ended; we say that "anything above this version is 
supposed to work", even though we have not tested with gcc 8 or 9. The 
alternative is to say that "we've tested this between gcc 4.7 and 7.3 
and will only support it for this version span", but that is in most 
cases more likely to break when gcc 8 comes along.



Because there are various linkers in play (old GNU ld, gold, lld, 
macosx ld, BSD ld) and we'd like our compilers to work the same way on 
various platforms, I'm vaguely unhappy with 
TOOLCHAIN_MINIMUM_LD_VERSION_gcc. It looks 
like TOOLCHAIN_EXTRACT_LD_VERSION is another place where we conflate 
toolchains and operating systems.


The linker is often in a situation where it's conflated between 
toolchain and operating system. :-) I think it's more due to how the 
linker is positioned/perceived, than due to a fault of ours. On Windows, 
for instance, the linker is clearly a part of the toolchain. On solaris, 
there's a system linker and the solstudio does not distribute any linker 
on their own. On linux, the gcc toolchain actually allows you to select 
linker (gold, etc), but arguably they belong to the gcc toolchain rather 
than the OS.



We can add a similar check for the clang toolchain, if you want.

Mixing and matching compilers and linkers willy-nilly is not a
supported build option


As always, I am for portability and for toolchain competition.  I'd 
like to be able to build with Intel's icc toolchain.


That might be a noble goal, but it's not realistic. There's a huge cost 
involved with supporting different combinations; the combinatorial 
matrix blows up quickly, and if these different combinations are not 
regularly tested, they *will* break.


That being said, I'm too in general in favor of supporting more rather 
than less. But only as long as there's no high price to pay for the 
common/reasonable combinations.


/Magnus

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Magnus Ihse Bursie]

On 2018-09-11 18:14, Severin Gehwolf wrote:

Hi Erik,

Thanks for the review!

On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:

Hello Severin,

Even if using the macro, I still think you need to add a condition on
the compiler types where the switch can be reasonably expected to exist.

How about this?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/

Looks good to me, too.

Thanks for keeping the spirit high despite the high number of respins of 
the patch!


/Magnus



Thanks,
Severin


On 2018-09-11 05:02, Severin Gehwolf wrote:

On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:

I see. I was not aware of that issue, but we clearly need to file a bug
for it and fix it. In this case I think it's fine to us the macro however.

OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/

Micro-benchmark results from running [1] for x86_64 and ppc64le are
here (-O2 is sufficient it seems):

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

More thoughts?

Thanks,
Severin

[1] https://github.com/gromero/strictmath/

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Magnus Ihse Bursie]

On 2018-09-12 12:19, Magnus Ihse Bursie wrote:



On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Oh, and there's a typo in CLDRConverter.java: " Note: the entries are 
alphabetically soreted, *except* the "world" region".


/Magnus

Re: RFR: JDK-8210519: build/releaseFile/CheckSource.java failed additional sources found

[Magnus Ihse Bursie]

On 2018-09-11 20:39, Erik Joelsson wrote:

Hello,

I do agree with your points.

http://cr.openjdk.java.net/~erikj/8210519/webrev.02/


Looks good to me.

/Magnus




On 2018-09-11 11:32, Mikael Vidstedt wrote:

Looks good, thanks for fixing.

Arguably the ":((hg)|(git)):[a-z0-9]*\\+?” string could be a constant 
(re-)used in the two places it occurs, and the nested if statements 
inside checking the Oracle specific part could be turned around to 
check "if (isOpenJDK)” first to avoid the negation, but that’s just 
my preference.


Cheers,
Mikael

On Sep 10, 2018, at 3:09 PM, Erik Joelsson 
 wrote:


When I added support for git as SCM in the build, I forgot to update 
the test that verifies the release file contents. This patch updates 
the test to also look for hg/git in the SOURCE strings. While there 
I also made the test more strict on the format and less strict when 
run against non Oracle produced builds where we probably shouldn't 
be making assumptions on what extra repositories may be involved and 
included in the SOURCE line.


Bug: https://bugs.openjdk.java.net/browse/JDK-8210519

Webrev: http://cr.openjdk.java.net/~erikj/8210519/webrev.01/

/Erik

Re: [12]: RFR: Use CLDR's time zone mappings for Windows

[Magnus Ihse Bursie]

On 2018-09-10 23:34, Naoto Sato wrote:

Hello,

Please review the fix to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8209167

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8209167/webrev.01/


Some comments:
In make/copy/Copy-java.base.gmk:
+ifneq ($(findstring $(OPENJDK_TARGET_OS), aix),)

The findstring construct is hard to read and only needed when we test 
more than one OS. Please rewrite as a single ifeq test for aix.


In GensrcCLDR.gmk:
I don't understand the CLDR_WINTZMAPPINGS file? There's no rule to 
generate it, there's just a dependency..?


The removal of the duplicate "common", that seems like a separate bug fix?

/Magnus



This fix is to remove the hand crafted map file (lib/tzmappings) on 
Windows, which maps Windows time zones to Java's tzid. It is now 
dynamically generated at the build time, using CLDR's data file 
(windowsZones.xml)


Naoto

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

On Wed, 2018-09-12 at 17:58 +1000, David Holmes wrote:
> But I don't understand why the optimization setting is being tied to the 
> availability of the -ffp-contract flag?

In configure we perform a check for gcc or clang whether that flag is
supported. If it is, it would be non-empty exactly having -ffp-contract 
as value. It could be another set of flags for other arches if somebody
wanted to do the same, fwiw. In JDK 8, for example, it's "-mno-fused-
madd -fno-strict-aliasing" for ppc64:
http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/2660b127b407/make/lib/CoreLibraries.gmk#l63

We need support for that flag (or a set of flags) when we optimize
fdlibm since otherwise we would lose precision. If the flag is empty
we'd not optimize as we can't guarantee precision. That's why we tie
optimization to the availability of that flag. The expectation is for
this flag to be available on gcc/clang arches only at this point. Does
that make sense?

Thanks,
Severin

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[David Holmes]

Hi Severin,

On 12/09/2018 5:48 PM, Severin Gehwolf wrote:

Hi David,

On Wed, 2018-09-12 at 13:57 +1000, David Holmes wrote:

Hi Severin,

Sorry I'm a bit confused now.

Originally for ppc/s390x/aarch64 the optimization level for fdlibm was
HIGH. But now it will be LOW due to:

45 ifneq ($(FDLIBM_CFLAGS), )
46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW

as those platforms will use gcc/clang with support for -ffp-contract and
so FDLIBM_CFLAGS will not be empty.

??


Correct. That was done based on Andrew Haley's comment here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023160.html

I've done some measurements with -O2 and -O3. -O2 is sufficient for a
2.75 speedup for sin/cos on ppc64le as compared to the -O0 baseline. On
the other hand the speedup of -O3 as compared to -O2 is only 1.05 for
sin/cos on ppc64le.

Since the difference between them were not huge, I've used -O2, a.k.a
LOW. See also:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

To me it seems -O2 is sufficiently good. Note that HIGH == -O3 and LOW
== -O2 on gcc arches. Does that clear things up?


Yes that explains the missing -O2 - thanks.

But I don't understand why the optimization setting is being tied to the 
availability of the -ffp-contract flag?


Thanks,
David


FWIW, I'm happy to change it back to HIGH if there are concerns. See
also Gustavo's reply here for some more data points:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023193.html

Thanks,
Severin


David

On 12/09/2018 2:14 AM, Severin Gehwolf wrote:

Hi Erik,

Thanks for the review!

On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:

Hello Severin,

Even if using the macro, I still think you need to add a condition on
the compiler types where the switch can be reasonably expected to exist.


How about this?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/

Thanks,
Severin


On 2018-09-11 05:02, Severin Gehwolf wrote:

On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:

I see. I was not aware of that issue, but we clearly need to file a bug
for it and fix it. In this case I think it's fine to us the macro however.


OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/

Micro-benchmark results from running [1] for x86_64 and ppc64le are
here (-O2 is sufficient it seems):

http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

More thoughts?

Thanks,
Severin

[1] https://github.com/gromero/strictmath/

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

On Tue, 2018-09-11 at 10:30 -0700, Erik Joelsson wrote:
> Looks good, thanks!

Thanks for the review, Erik.

We've ran JCK 11 on this patch which passed on our end. I'll wait a few
more days whether there are objections and then push it.

Thanks,
Severin

> /Erik
> 
> 
> On 2018-09-11 09:14, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > Thanks for the review!
> > 
> > On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Even if using the macro, I still think you need to add a
> > > condition on
> > > the compiler types where the switch can be reasonably expected to
> > > exist.
> > 
> > How about this?
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/
> > 
> > Thanks,
> > Severin
> > 
> > > On 2018-09-11 05:02, Severin Gehwolf wrote:
> > > > On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:
> > > > > I see. I was not aware of that issue, but we clearly need to
> > > > > file a bug
> > > > > for it and fix it. In this case I think it's fine to us the
> > > > > macro however.
> > > > 
> > > > OK. Update webrev, which now uses
> > > > FLAGS_COMPILER_CHECK_ARGUMENTS.
> > > > 
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/
> > > > 
> > > > Micro-benchmark results from running [1] for x86_64 and ppc64le
> > > > are
> > > > here (-O2 is sufficient it seems):
> > > > 
> > > > 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/
> > > > 
> > > > More thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] https://github.com/gromero/strictmath/
> > > > 
> 
> 

Re: RFR: 8210416: [linux] Poor StrictMath performance due to non-optimized compilation

[Severin Gehwolf]

Hi David,

On Wed, 2018-09-12 at 13:57 +1000, David Holmes wrote:
> Hi Severin,
> 
> Sorry I'm a bit confused now.
> 
> Originally for ppc/s390x/aarch64 the optimization level for fdlibm was 
> HIGH. But now it will be LOW due to:
> 
>45 ifneq ($(FDLIBM_CFLAGS), )
>46   BUILD_LIBFDLIBM_OPTIMIZATION := LOW
> 
> as those platforms will use gcc/clang with support for -ffp-contract and 
> so FDLIBM_CFLAGS will not be empty.
> 
> ??

Correct. That was done based on Andrew Haley's comment here:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023160.html

I've done some measurements with -O2 and -O3. -O2 is sufficient for a
2.75 speedup for sin/cos on ppc64le as compared to the -O0 baseline. On
the other hand the speedup of -O3 as compared to -O2 is only 1.05 for
sin/cos on ppc64le.

Since the difference between them were not huge, I've used -O2, a.k.a
LOW. See also:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/

To me it seems -O2 is sufficiently good. Note that HIGH == -O3 and LOW
== -O2 on gcc arches. Does that clear things up?

FWIW, I'm happy to change it back to HIGH if there are concerns. See
also Gustavo's reply here for some more data points:
http://mail.openjdk.java.net/pipermail/build-dev/2018-September/023193.html

Thanks,
Severin

> David
> 
> On 12/09/2018 2:14 AM, Severin Gehwolf wrote:
> > Hi Erik,
> > 
> > Thanks for the review!
> > 
> > On Tue, 2018-09-11 at 08:57 -0700, Erik Joelsson wrote:
> > > Hello Severin,
> > > 
> > > Even if using the macro, I still think you need to add a condition on
> > > the compiler types where the switch can be reasonably expected to exist.
> > 
> > How about this?
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.05/
> > 
> > Thanks,
> > Severin
> > 
> > > On 2018-09-11 05:02, Severin Gehwolf wrote:
> > > > On Mon, 2018-09-10 at 09:29 -0700, Erik Joelsson wrote:
> > > > > I see. I was not aware of that issue, but we clearly need to file a 
> > > > > bug
> > > > > for it and fix it. In this case I think it's fine to us the macro 
> > > > > however.
> > > > 
> > > > OK. Update webrev, which now uses FLAGS_COMPILER_CHECK_ARGUMENTS.
> > > > 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/webrev.04/
> > > > 
> > > > Micro-benchmark results from running [1] for x86_64 and ppc64le are
> > > > here (-O2 is sufficient it seems):
> > > > 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8210416/microbenchmarks_results/
> > > > 
> > > > More thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > [1] https://github.com/gromero/strictmath/
> > > > 
> > > 
> > >