RFR(S): 8028471: PPC64 (part 215): opto: Extend ImplicitNullCheck optimization.

[Lindenmaier, Goetz]

Hi Vladimir,

ok, I see, narrow_oop_use_implicit_null_checks() is set as one would
expect: at most true in heapbased mode.

I removed the check for UseCompressedOops.

In our VM, narrow_oop_use_implicit_null_checks() can be true in unscaled
mode.  I didn't contribute that as it requires too much shared changes.

> My question was, is that what you want for AIX? Because you have next 
> comment:
> "On AIX, no such memory operands exist, so we need not check for them."
With 'need not', I mean that I must not implement it.  On other platforms, 
a lot of opportunities for null checks could be lost, so I put the Unimplemented()
there.  I adapted the comment:
      // returns true everywhere else. On PPC, no such memory operands
      // exist, therefore we did not yet implement a check for such operands.

I updated the webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8028471-0-zpage/

Best regards,
  Goetz.

-----Original Message-----
From: ppc-aix-port-dev-bounces at openjdk.java.net [mailto:ppc-aix-port-dev-bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
Sent: Thursday, November 21, 2013 8:48 PM
To: 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
Subject: Re: RFR(S): 8028471: PPC64 (part 215): opto: Extend ImplicitNullCheck optimization.

On 11/21/13 12:30 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
>> Without (UseCompressedOops && Universe::narrow_oop_base() > 0) check
>> Universe::narrow_oop_use_implicit_null_checks() is useless because it is
>> 'true' by default. That is why I asked to move check inside to combine
>> them in one place.
> Yes, it is useless, but it will not be evaluated.  And I need it if narrow_oop_base()
> is set.
> But is it really true by default? See arguments.cpp:

It is initialized by 'true':

NarrowPtrStruct Universe::_narrow_oop = { NULL, 0, true };

except special case for Win64:

> #ifdef _WIN64
>      if (UseLargePages && UseCompressedOops) {
>        // Cannot allocate guard pages for implicit checks in indexed addressing
>        // mode, when large pages are specified on windows.
>        // This flag could be switched ON if narrow oop base address is set to 0,
>        // see code in Universe::initialize_heap().
>        Universe::set_narrow_oop_use_implicit_null_checks(false);
>      }
> #endif //  _WIN64

which can be reset back to 'true' if 0 page is used for traps:

       Universe::set_narrow_oop_base(NULL);
       // Don't need guard page for implicit checks in indexed
       // addressing mode with zero based Compressed Oops.
       Universe::set_narrow_oop_use_implicit_null_checks(true);

>
>> You can use Matcher::gen_narrow_oop_implicit_null_checks() check
>> instead.
> No, that returns true if narrow_oop_use_complex_address() is set.

But you said narrow_oop_use_complex_address() is false on AIX:

"> but ppc has no loads that support this."

Did I get it wrong? If it could be true then yes, we can't use it.

> I must know whether the page before the heapbased heap is protected.
> There should be Universe::heapbased_base_page_is_protected(),
> that would be much more descriptive.
>
>> Note Universe::narrow_oop_base() != NULL only if
>> UseCompressedOops is true.
> Yes, you mean I should remove the test for UseCompressedOops?

Yes.

>
>>> No ShouldNotReachHere for AIX.  I tried to improve the comment
>>> instead.
>> So you want to return false if it is not DecodeN. Right?
> No, if it's not DecodeN_NN:
>
>      xn = loadn
>      xo = xn == 0 ? 0 : xn<<3+base  // DecodeN
>      loadI (xo+offset)
> will not trap, as it accesses the zero page.
>
>      xn = loadn
>      xo = xn<<3+base  // DecodeN_NN
>      loadI (xo+offset)
> will trap, as it accesses the base page of the heapbased heap.
>
> If you match the decode into the memory operand of the second
> Load, this check will not work, therefore I put the unimplemented()
> there:
>      xn = loadn
>      loadI (xo<<3+base+offset)
> but ppc has no loads that support this.
>
> There is no obvious, platform independent way to distinguish
> DecodeN_NN and DecodeN.

You may be misunderstood me (or I you :) ).

This code is only executed for AIX: (!os::zero_page_read_protected()). 
So I am fine to have NOT_AIX(Unimplemented()) because our current 
platforms will not hit it.

My question was about AIX. If Op_DecodeN and following NotNULL checks 
are false you return 'false' from accesses_heap_base_zone() and EXPLICIT 
check will be generated.

My question was, is that what you want for AIX? Because you have next 
comment:

"On AIX, no such memory operands exist, so we need not check for them."

Which I read as AIX only have case with DecodeN so above checks should 
always succeed.  That is why I am confused and asked if you need 
ShouldNotReachHere there.

Thanks,
Vladimir

>
> Thanks for this detailed review!
> Best regards,
>    Goetz.
>
>
>
> Thanks,
> Vladimir
>
>>
>> Sorry for the wrong indents.
>>
>> Best regards,
>>     Goetz.
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Wednesday, November 20, 2013 9:04 PM
>> To: Lindenmaier, Goetz; 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
>> Subject: Re: RFR(S): 8028471: PPC64 (part 215): opto: Extend ImplicitNullCheck optimization.
>>
>> Hi, Goetz
>>
>> Can you move all checks in lcm.cpp into new method and rename it to
>> needs_explicit_null_check_for_read()? So the condition will be simple:
>>
>> if (!was_store && needs_explicit_null_check_for_read(val)) {
>>      continue;
>> }
>>
>> You should also add ShouldNotReachHere() for AIX near Unimplemented()
>> for non AIX.
>>
>> And, please, fix indents in new method.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/20/13 2:11 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> ImplicitNullChecks did not work on platforms where the zero
>>> page is only write protected.
>>>
>>> This change adds os property 'zero_page_read_protected' and extends
>>> the ImplicitNullCheck optimization to consider only stores if
>>> this property is not true. If a decoded compressed oop will access the
>>> guard page before the heap, Loads work again.
>>> This is needed on AIX, where the page at address '0' is only write-protected.
>>>
>>> Please review and test this change:
>>> http://cr.openjdk.java.net/~goetz/webrevs/8028471-0-zpage/
>>>
>>> I tagged this S, as a majority of the patches apply to the ppc platform files.
>>>
>>> Best regards,
>>>      Goetz.
>>>