Redundant dereferences in C2 shenandoah_wb?

[Aleksey Shipilev]

AFAIU, implicit NPE checks are prepared on a higher level, e.g. at
GraphKit::null_check, which sets up uncommon_trap with appropriate
dependencies. Placing a lowered memory access in .ad does not yield an
implicit null check, this is just a potential SEGV.

Anyhow, I see the null check before emitting the ShenandoahWriteBarrier
in GraphKit::shenandoah_write_barrier:

  Node* not_null_obj = null_check_oop(obj, &null_ctrl);
  ...
  Node* n = shenandoah_write_barrier_helper(*this, not_null_obj, adr_type);

...

static Node* shenandoah_write_barrier_helper(GraphKit& kit, Node* obj,
const TypePtr* adr_type) {
  ShenandoahWriteBarrierNode* wb = new
ShenandoahWriteBarrierNode(kit.control(), kit.memory(adr_type), obj);
  ...
}

Therefore it seems we already have null pointer checks at the high-level
IR before the barrier, and having a dereference in barrier low-level
encoding still feels redundant. I must be missing something!

Thanks,
-Aleksey

On 09/07/2016 02:01 PM, Roman Kennke wrote:
> The compiler can replace explicit null checks with implicit null
checks, and rely on the first instruction of the wb (or any load or
store for that matter) blowing a SIGSEGV. I suppose this happens when
the compiler figures that null is rare or impossible.
> 
> RomanAm 07.09.2016 12:41 nachm. schrieb Aleksey Shipilev <ashipile at redhat.com>:
>>
>> Wait a minute, can you elaborate the scenario where we want to intercept 
>> null there? Because this code: 
>>
>>     @Benchmark 
>>     @CompilerControl(CompilerControl.Mode.DONT_INLINE) 
>>     public void storeToNull() { 
>>         try { 
>>             targetNull.field = null; 
>>         } catch (NullPointerException e) { 
>>             // expected 
>>         } 
>>    } 
>>
>>     static class Target { 
>>         Object field; 
>>     } 
>>
>> ...triggers NPE much earlier (look for "WHY THIS CHECK?"): 
>> http://cr.openjdk.java.net/~shade/shenandoah/wb.deref.1/storeToNull.perfasm 
>>
>> How could that check fire a SEGV, if we already tested %r11 is not null? 
>> I assume this is universal, and we nullcheck before doing shenandoahWB code? 
>>
>> Thanks, 
>> -Aleksey 
>>
>> On 09/07/2016 01:10 PM, Roman Kennke wrote: 
>>> The first is only there to trigger a SIGSEGV/NPE on null. The 2nd 
>>> absolutely needs to come after the check for evacuation in progress 
>>> to avoid a load load race at the end of evacuation. I don't think it 
>>> affects performance much. If you have a solution to trigger NPE 
>>> correctly on the 2nd instruction and not only the first (e.g. improve 
>>> the signal handling), that would be very welcome! 
>>>
>>> Cheers, RomanAm 07.09.2016 12:01 nachm. schrieb Aleksey Shipilev 
>>> <ashipile at redhat.com>: 
>>>>
>>>> Hi, 
>>>>
>>>> I am studying the generated code for Shenandoah barriers. These two 
>>>>   dereferences seem excessive: 
>>>> http://cr.openjdk.java.net/~shade/shenandoah/wb.deref.1/webrev.00/ 
>>>>
>>>>
>>>> C1 and interpreter code for both x86_64 and AArch64 do not seem to 
>>>> do the additional read barrier before testing 
>>>> evacuation_in_progress, doing that only after. Am I missing 
>>>> something? 
>>>>
>>>> Thanks, -Aleksey