RFR: Load reference barriers

[Roman Kennke]

Hi Aleksey,

> On 2/14/19 5:45 PM, Roman Kennke wrote:
>> Webrev:
>> http://cr.openjdk.java.net/~rkennke/load-ref-barriers/webrev.00/
> 
> Whoa, impressive stuff!
> 
> What's the plan with this? I say push it to sh/jdk, let it pass testing, stabilization, cleanups,
> and then proposed it upstream week(s) later? This would give us automatic webrevs against upstream,
> the clear binaries for comparison (basically, jdk/jdk vs sh/jdk) and the patch would not bitrot as much.

Oh yeah, forgot about that. Yes, that would be a good plan.

> I have eyeballed generated code on some interesting benchmarks and it looks clean. It also seems to
> pass hotspot_gc_shenandoah for me.
> 
> Comments:
> 
>  *) Does this mean we execute LRB before SATB enqueue? That means, SATB drainers in
> concurrent/traversal may skip the LRB resolution?

No, unfortunately not. We load the previous value from heap, which *may*
have from-space ptrs.

>  *) There is new include in .ad, why?
>      26 #include "gc/shenandoah/c2/shenandoahSupport.hpp"

Leftover. Removed.

>  *) Why _claim_strong here?
> 
>    class ShenandoahEvacuateUpdateRootsTask : public AbstractGangTask {
>      ...
>       CLDToOopClosure clds(&cl, ClassLoaderData::_claim_strong);

The real question is, why is it there at all? The RP doesn't need it.
It's a leftover from a previous experiment, I removed it and all the CLD
machinery there.

>  *) Looking at ShenandoahRootEvacuator::process_evacuate_roots: we are now pre-evacuating most roots
> now? Is this to avoid putting LRB when accessing those roots from VM code? I thought this would be
> handled by barrier set.

This warrants some explanation: we must ensure the strong invariant
there. Which generally means we would need to pre-evac all roots.
However, some roots are also protected by the _not_in_heap accessors,
i.e. there's an LRB at loads from those roots. I excluded those roots
from pre-evacuation. The remaining roots are threads, code (as was
before) plus a bunch of cheap ones like universe, synchronizer etc,
those shouldn't hurt. In-fact, the roots that have barriers on them
could be done concurrently, but this is a whole project of its own.

>  *) Debugging leftover in node.cpp?
> 
>   86     if (Compile::current()->method() != NULL) {
>   87       os::message_box("xxx", "yyy");
>   88     }

Yep.

>  *) phasetype.hpp: we would need better name for PHASE_CUSTOM...

That is also leftover and I removed it.

> There are minor corrections from my side, here:
>   http://cr.openjdk.java.net/~shade/shenandoah/lrb-shade-fixes.patch

Thanks!! I noticed you removed ShRB from formssel. We can actually also
remove ShLRB from there, because this is fully expanded in ideal graph,
and no need to match it in .ad.

Another advantage that I forgot to mention in original RFR:
- We can dispense with the madness of memory dependencies between
barriers. It *would* be possible to generate two different strength
LRBs: one kinda like RBs and one kinda like WBs, and if we did, we'd
need the dependencies, but looking at some debug output, it doesn't seem
worth. There's just soooo few cases where LRBs would actually lower to
WEAK instead of STRONG, and perf-wise I am not sure it's noticable.

Incremental diff:
http://cr.openjdk.java.net/~rkennke/load-ref-barriers/webrev.01.diff/
Full diff:
http://cr.openjdk.java.net/~rkennke/load-ref-barriers/webrev.01/

Good now?

I'd also like Roland to ack the C2 changes.

Roman