RFR: JDK-8210656: Object equals abstraction for BarrierSetC2

[Roman Kennke]

Hi Erik,

> I'm glad this idea works well for you. If you need an Ideal hook for
> CmpPNode anyway for barrier optimizations, then I suppose we should sort
> something out there. In that API though, it would be great if it was not
> specific to CmpPNode. I'm thinking something along the lines of Node*
> BarrierSetC2::ideal_node(Node* n), and then figure out if something
> should or should not be done for a given node in the backend. That way,
> if we need ideal hooks for other node types, we could reuse the same API
> to ask the BarrierSetC2 if it has more ideal nodes. What do you think?


Yeah, that would actually be good. We have at least one more place that
I know of where we hook into Ideal(). The API would be something like this:

Node* ideal_node(PhaseGVN* phase, Node* n, bool can_reshape) const;

And a sample usage from CallLeafNode would look like this:

Node* CallLeafNode::Ideal(PhaseGVN* phase, bool can_reshape) {
  Node* ideal =
BarrierSet::barrier_set_c2()->barrier_set_c2()->ideal_node(phase, n,
can_reshape);
  if (ideal != NULL) {
    return ideal;
  }
  return CallNode::Ideal(phase, n, can_reshape);
}

Unfortunately (or maybe fortunately) this can't be inserted generically
into Node::Ideal(..) because subclasses can't be expected to always call
the super implementation.

Thanks for reviewing! I'll withdraw this RFR and push the additional
resolve() hooks via another RFE.

Cheers,
Roman


> Thanks,
> /Erik
> 
> On 2018-09-13 14:15, Roman Kennke wrote:
>> Hi Erik,
>>
>> I talked to Roland about this. It turns out that we already have this
>> optimization pass, and could just as well live with cmp(resolve(a),
>> resolve(b)). We need a little (shenandoah-specific-) hook in
>> CmpPNode::Ideal() for that though (but we'd need that anyway I suppose).
>> If you agree with that, I'll withdraw this RFR. Ok?
>>
>> Roman
>>
>>> Hi Roman,
>>>
>>> Interesting. So semantically, this is cmp(resolve(a), resolve(b)), but
>>> you have circumstances in which the barriers are unnecessary and can be
>>> elided. Any of them having null in their type is one reason, but I
>>> suppose there are surely other reasons as well (such as finding
>>> dominating write barriers).
>>>
>>> I see two different approaches for this barrier elision:
>>> 1) Elide it during parsing (as you propose)
>>> 2) Elide it during Optimize (which I think conceptually looks like a
>>> natural fit)
>>>
>>> I originally proposed a function on BarrierSetC2 that I think I called
>>> optimize_barriers() or something like that. The idea was to use this
>>> hook to let GC barrier code shave off pointless (not to be confused with
>>> useless) barriers that can be removed. Roland thought that this seemed
>>> too specific to ZGC to warrant a general API, and I agreed, because
>>> indeed only ZGC used this hook at the time. This is today
>>> ZBarrierSetC2::find_dominating_barriers which is called straight from
>>> Optimize.
>>>
>>> I wonder if it would make sense to re-instate that hook. Then you could
>>> use the existing resolve() barriers during parsing, and leave barrier
>>> elision tricks (null checks included, plus other tricks you might have
>>> up your sleeve) to Optimize. For example, you might be able to walk your
>>> list of barriers and disconnect these pointless barriers. What do you
>>> think?
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2018-09-12 22:11, Roman Kennke wrote:
>>>> This introduces an abstraction to deal with object equality in
>>>> BarrierSetC2. This is needed by GCs that can have different copies of
>>>> same objects alive like Shenandoah.
>>>>
>>>> The approach chosen here is slightly different than we did in e.g.
>>>> BarrierSetAssembler and the runtime Access API: instead of owning the
>>>> whole equality, it only provides a resolve-like method to resolve the
>>>> operands to stable values. The reason for doing this is that it's
>>>> easier
>>>> to do this way in intrinsics if those barriers are detached from the
>>>> actual CmpP. This is because the barriers create new memory states, and
>>>> we'd have to create memphis around those things, which is considerably
>>>> more complex.
>>>>
>>>> I chose to add a new resolve_for_obj_equals(a, b) method instead of
>>>> using two calls to resolve(a); resolve(b); because this allows for
>>>> optimization: if any of a or b is known to be NULL, we can elide
>>>> barriers for both. This is not possible to do with two independent
>>>> resolve() calls.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8210656
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~rkennke/JDK-8210656/webrev.00/
>>>>
>>>> Testing: passes hotspot/jtreg:tier1
>>>>
>>>> What do you think about this?
>>>>
>>>> Thanks,
>>>> Roman
>>>>
>>
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20180913/ec337e6a/signature.asc>