RFR(M): 8212243: More gc interface tweaks for arraycopy and some other smaller changes in preparation for Shenandoah

[Erik Österlund]

Hi Roland,

On 2018-10-18 09:43, Roland Westrelin wrote:
> Hi Erik,
>
> Thanks for looking at this.
>
>> One goal I have had in the barrier set classes is to not introduce the
>> concepts of pre/post (write) barriers until the ModRefBarrierSet layer,
>> as it is not a concept that is at all shared for our GCs. Therefore, I
>> would prefer if this:
>>
>> virtual void array_copy_post_barrier_at_expansion(ArrayCopyNode* ac,
>> Node*& c, Node*& m, PhaseIterGVN& igvn) const { } ...to all be moved
>> into ModRefBarrierSetC2. I'm imagining to on BarrierSetC2 only expose
>> something like clone_barrier_at_expansion() that gets overridden by
>> ModRefBarrierSetC2 to do the check if post_barriers are needed, and then
>> call the "array_copy_post_barrier_at_expansion" member function of
>> ModRefBarrierSetC2, that looks like it should also be renamed to
>> clone_post_barrier_at_expansion.
> That wouldn't work. The reason for this change is Shenandoah and
> ShenandoahBarrierSetC2 is a subclass of BarrierSetC2. I suppose it's not
> really a post barrier eventhough other gcs could use that hook as a way
> to put their post barrier. We use it to fix references after the copy is
> done.

That seems to lead us to the next question... is there a reason why 
ShenandoahBarrierSetC2 is not a subclass of ModRefBarrierSetC2? Since it 
e.g. uses a SATB barrier, that seems like it would fit more naturally, 
and would remove the need for all this. If there are things in 
ModRefBarrierSetC2 you do not need, then you can simply opt out from 
those things by overriding the virtual member functions.

>> virtual Node* array_copy_load_store_barrier(PhaseGVN *phase, bool
>> can_reshape, Node* v, MergeMemNode* mem, Node*& ctl) const { return v; }
>> ...I would prefer to pass in the BasicType, and perform the T_OBJECT
>> filtering inside, for symmetry with other similar hooks. Also it beats
>> me that this is strictly speaking a load barrier for loads performed in
>> arraycopy. Would it be possible to use something like access_load_at
>> instead? That would have been fantastic. Or is there a mis-fit, such as
>> not having a GraphKit with memory state? If so, it would be interesting
>> to see what it would take to fix that. I believe that hook could be
>> useful for ZGC cloning as well. Thanks, /Erik
> GraphKit is a parse time only thing. So the existing gc interface
> doesn't offer any way to add barriers once parsing is over. This code
> runs after parsing in optimization phases. In the valhalla repo, Tobias
> made a change so we can create a GraphKit at optimization time at places
> where we have jvm state. So bringing that would allow the use of
> access_load and friends in the arraycopy code. Except it's not a simple
> change so I would rather go with what we have here and rework it down
> the road.

Right. I placed the parse-time access_load_at frontend in GraphKit, 
because it was only ever used at parse time.
But it would appear that it would have been great to have something like 
that available after parse time as well. I do understand though that 
getting a GraphKit in here Tobias style, is not a quick fix, and is out 
of the question for this RFE.

So currently the context information of the access comes in two 
flavours: C2Access for context information that is shared for all 
accesses, and C2AtomicAccess with extra information only used by 
atomics. I imagine we could put a new type of access context information 
wrapper into that hierarchy to fit this use case.

So basically, you could make a new C2PostParseTimeAccess (name?!) that 
you wrap this information in, and send it through the 
BarrierSetC2::load_at backend.
Similar how there is a GraphKit::access_load_at helper for baking the 
context object, you can have a helper (somewhere at your convenience) 
for baking the C2PostParseTimeAccess.

How would that feel to you?

Thanks,
/Erik

> Roland.