Some C2 fixes, cleanup and improvements to handling of write barriers

[Roman Kennke]

Hi Roland,

This is excellent! Please go ahead and push it.

Cheers,
Roman

Am Dienstag, den 21.06.2016, 11:19 +0200 schrieb Roland Westrelin:
> http://cr.openjdk.java.net/~roland/write-barrier-loopopts/webrev.00/
> 
> The main change here is that looking for a dominating memory barrier
> now
> also happens during loop optimizations. It's both a performance
> optimization (because it can handle 2 barriers that are far from each
> others) and a fix. The current code handles the following example:
> 
> if (..) {
>   write barrier for obj
> } else {
>   read barrier for obj
> }
> // memory phi here that merges memory proj from the write barrier and
> // memory state before the if
> 
> by simply replacing the read barrier with the write barrier. Then the
> write barrier is before the if and it affects the memory state of
> both
> branches so the phi after the if/else should be updated as well. But
> the
> current code fails to do that.
> 
> This change fixes that problem by:
> 
> 1) At IGVN time, only handling write barriers that dominates (and not
> write barriers whose memory dominates)
> 
> 2) At loop opts time, handling write barriers whose memory dominates
> and
> fixing the memory graph if required
> 
> I also changed ShenandoahBarrierNode::dominates_memory_impl and
> ShenandoahReadBarrierNode::dominates_memory_rb_impl so they are no
> longer recursive (to not blow the stack up) as is usual practice in
> c2
> code. Because dominating write barriers are now also handled in loop
> optimizations I set a depth limit for
> ShenandoahBarrierNode::dominates_memory_impl. IGVN transformations
> are
> expected to only look around the node being processed and be applied
> frequently so they should stay inexpensive. I also added an argument
> to
> ShenandoahBarrierNode::dominates_memory_impl and
> ShenandoahReadBarrierNode::dominates_memory_rb_impl so during parsing
> we
> don't cross Phis which I don't think is safe.
> 
> I also dropped the change to phaseX.cpp. I don't think that change
> would
> be accepted upstream. Value is also called during PhaseCCP and so a
> similar hack would have been required
> there. ShenandoahBarrierNode::Value() now drops the constant part of
> the
> type which should lead to the same result in the end because Identity
> already handles the constant barrier case and causes the barrier to
> go
> away.
> 
> When a write barrier can be removed, to have its memory projection go
> away, rather than drop the projection:
> 
> Node* ShenandoahWBMemProjNode::Identity(PhaseGVN* phase) {
> 
> ...
> 
>    if (wb->as_ShenandoahBarrier()->Identity_impl(phase) != wb) {
>      // If the parent write barrier would go away, make this mem proj
> go away first.
>      // Poke parent to give it a chance to go away too.
>      phase->igvn_rehash_node_delayed(wb);
>      return wb->in(ShenandoahBarrierNode::Memory);
>    }
> 
> and then expects:
> 
> Node* ShenandoahBarrierNode::Identity(PhaseGVN* phase) {
> 
>   Node* replacement = Identity_impl(phase);
>   if (replacement != this) {
>     // If we have a memory projection, we first need to make it go
> away.
>     Node* mem_proj = find_out_with(Op_ShenandoahWBMemProj);
>     if (mem_proj != NULL) {
>       phase->igvn_rehash_node_delayed(mem_proj);
>       return this;
>     }
>   }
>   return replacement;
> }
> 
> to cause the barrier to be optimized out. I simply made
> ShenandoahWriteBarrierNode::Identity() kill the projection. The risk
> I
> see in the current code is that ShenandoahWBMemProjNode::Identity()
> sees
> the barrier can be dropped and makes the projection go away, enqueues
> the barrier so it's removed, then some other nodes are processed by
> igvn
> that for some reason ressurects the barrier or makes the
> Identity_impl()
> call not find a replacement and finally
> ShenandoahBarrierNode::Identity() is called, the barrier doesn't go
> away
> but it doesn't have a projection any more.
> 
> Roland.
>