RFR(L): 8186027: C2: loop strip mining

[Nils Eliasson]

Hi Roland,

Sorry for the delay.

First - It's a very impressive work you have done!

Currently your patch doesn't apply cleanly. The fix of JDK-8189067 
changes loopopts.cpp.

I have run your code (based on jdk10 before JDK-8189067) through 
testing. I encountered a minor build problem on solaris_x64 (patch 
below), otherwise it was stable with no encountered test failures.

I have also run performance testing with the conclusion that no 
significant regression can be seen. In some benchmarks like 
scimark.sparse.large that has a known safepointing issue 
(https://bugs.openjdk.java.net/browse/JDK-8177704), very good results 
can be seen.

scimark.sparse.large using G1:
-XX:-UseCountedLoopSafepoints (default) ~86 ops/m
-XX:+UseCountedLoopSafepoints  ~106 ops/m
-XX:+UseCountedLoopSafepoints -XX:LoopStripMiningIter=1000 ~111 ops/m

The positive results leads us to the conclusion that we would like 
UseCountedLoopSafepoints to bedefault true, and LoopStripMiningIter 
default to 1000.

c2_globals.hpp:

-  product(bool, UseCountedLoopSafepoints, false,
+  product(bool, UseCountedLoopSafepoints, true,

-  product(uintx, LoopStripMiningIter, 0,
+  product(uintx, LoopStripMiningIter, 1000,

solaris_x64 complained about type conversion:

src/hotspot/share/opto/loopopts.cpp:
@@ -1729,7 +1729,7 @@
      Node* l = cl->outer_loop();
      Node* tail = cl->outer_loop_tail();
      IfNode* le = cl->outer_loop_end();
-    Node* sfpt = cl->outer_safepoint();
+    Node* sfpt = (Node*) cl->outer_safepoint();

src/hotspot/share/opto/opaquenode.cpp
@@ -144,7 +144,7 @@
    assert(iter_estimate > 0, "broken");
    if ((jlong)scaled_iters != scaled_iters_long || iter_estimate <= 
short_scaled_iters) {
      // Remove outer loop and safepoint (too few iterations)
-    Node* outer_sfpt = inner_cl->outer_safepoint();
+    Node* outer_sfpt = (Node*) inner_cl->outer_safepoint();

In the TraceLoopOpts print out I suggest changing space to underscore to 
conform with how the other print outs look:

"PreMainPost Loop: N153/N130 limit_check predicated counted [0,int),+1 
(26 iters) has_sfpt strip mined"

loopnode.cpp:1867
- tty->print(" strip mined");
+ tty->print(" strip_mined");


When your patch is updated, I will do some additional functional 
testing. Also, a second reviewer is required.


Best regards,

Nils Eliasson


On 2017-10-11 15:53, Roland Westrelin wrote:
>> I have started reviewing and testing I will sponsor your change when the
>> full review is completed.
> Thanks!
>
> Roland.