RFR(M): 8073480: C2 should optimize explicit range checks

[Roland Westrelin]

Hi Paul,

Answers inlined.

> I just re-verified that it works for me, and in addition works well with JDK-8003585.
> 
> The generated code for direct array access and Unsafe array access is now the same in my test.

Thanks for checking.

> Interestingly when i look at the generated code for unrolled loop blocks (again identical between array and Unsafe access) i still observe a signed conversion:

I noticed that one, too. I wonder where it’s coming from.

> 
> 0x0000000106a61852: add    0x10(%r9,%rbx,4),%eax
> 0x0000000106a61857: movslq %ebx,%r11
> 0x0000000106a6185a: add    0x14(%r9,%r11,4),%eax
> 0x0000000106a6185f: add    0x18(%r9,%r11,4),%eax
> 0x0000000106a61864: add    0x1c(%r9,%r11,4),%eax  ;*iadd
>                                              ; - arrays.ArrayTest::u_loop at 22 (line 65)
> 
> 0x0000000106a61869: add    $0x4,%ebx          ;*iinc
>                                              ; - arrays.ArrayTest::u_loop at 24 (line 64)
> 
> 0x0000000106a6186c: cmp    %r8d,%ebx
> 0x0000000106a6186f: jl     0x0000000106a61852  ;*if_icmpge
>                                              ; - arrays.ArrayTest::u_loop at 12 (line 64)
> 
> I dunno if that yields further clues for Alexey's use-case.  It's kind of funny. With your patch adding explicit bounds checks will now improve such cases :-)
> 
> Also interesting is if i switch from an Unsafe relaxed read to an Unsafe volatile read, then i observe:
> 
> 0x0000000106a0f960: movslq %edi,%r10
> 0x0000000106a0f963: add    0x10(%rbx,%r10,4),%eax
> 0x0000000106a0f968: mov    %edi,%r11d
> 0x0000000106a0f96b: add    $0x3,%r11d
> 0x0000000106a0f96f: mov    %edi,%r8d
> 0x0000000106a0f972: add    $0x2,%r8d
> 0x0000000106a0f976: movslq %r11d,%r10
> 0x0000000106a0f979: movslq %r8d,%r11
> 0x0000000106a0f97c: mov    %edi,%r8d
> 0x0000000106a0f97f: inc    %r8d
> 0x0000000106a0f982: movslq %r8d,%r8
> 0x0000000106a0f985: mov    0x10(%rbx,%r8,4),%r8d
> 0x0000000106a0f98a: mov    0x10(%rbx,%r11,4),%edx
> 0x0000000106a0f98f: mov    0x10(%rbx,%r10,4),%r10d  ;*invokevirtual getIntVolatile
>                                              ; - arrays.ArrayTest::u at 37 (line 90)
>                                              ; - arrays.ArrayTest::u_loop at 19 (line 80)
> 
> 0x0000000106a0f994: add    %r8d,%eax
> 0x0000000106a0f997: add    %edx,%eax
> 0x0000000106a0f999: add    %r10d,%eax         ;*iadd
>                                              ; - arrays.ArrayTest::u_loop at 22 (line 80)
> 
> 0x0000000106a0f99c: add    $0x4,%edi          ;*iinc
>                                              ; - arrays.ArrayTest::u_loop at 24 (line 79)
> 
> 0x0000000106a0f99f: cmp    %r9d,%edi
> 0x0000000106a0f9a2: jl     0x0000000106a0f960  ;*if_icmpge
>                                              ; - arrays.ArrayTest::u_loop at 12 (line 79)
> 
> Which is similar to the code generated, pre-patch, when there is no range check preceding the access.

Thanks for doing that many experiments and trying to see where it breaks!
My patch requires that the range check immediately dominates the memory access. With volatile accesses there’s a memory barrier in between. This:

diff --git a/src/share/vm/opto/ifnode.cpp b/src/share/vm/opto/ifnode.cpp
--- a/src/share/vm/opto/ifnode.cpp
+++ b/src/share/vm/opto/ifnode.cpp
@@ -958,8 +958,10 @@
           }
         }
       } else if (use->is_Mem()) {
-        Node* ctrl = use->in(0);
-        if (ctrl == fail) {
+        if (ctrl == fail ||
+            (ctrl->in(0)->is_Proj() &&
+             ctrl->in(0)->in(0)->is_MemBar() &&
+             ctrl->in(0)->in(0)->in(0) == fail)) {
           Node* init_n = stack.node_at(1);
           assert(init_n->Opcode() == Op_ConvI2L, "unexpected first node");
           Node* new_n = igvn->C->conv_I2X_index(igvn, l, array_size);

would allow the barrier. I think it’s safe.

Roland.

> 
> Paul. 
> 
> On Mar 12, 2015, at 6:34 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
>> Here is a new webrev for this:
>> 
>> http://cr.openjdk.java.net/~roland/8073480/webrev.01/
>> 
>> I took Vladimir’s comments into account (added test for null inputs in several places, strengthen the test to make sure a middle guard is a null check, renamed functions) and added code that look for a ConvI2L between the range check and a memory access that follows and annotate that ConvI2L with a tighter type so the movslq that Paul spotted are removed from the final code. 
>> 
>> Roland.
>