RFR: JDK-8203157: Object equals abstraction for BarrierSetAssembler

[Roman Kennke]

Ok, done here:

Incremental:
http://cr.openjdk.java.net/~rkennke/JDK-8203157/webrev.01.diff/
Full:
http://cr.openjdk.java.net/~rkennke/JDK-8203157/webrev.01/

Good now?

Thanks, Roman


> Hi Roman,
> 
> Sure. As long as there is no need for AS_RAW equals in the assembly
> code, we don't need to add it now.
> However, that means that there are currently no properties in the
> decorators we care about at the moment. Therefore, the decorator
> parameter of obj_equals should be removed; it serves no purpose.
> 
> Thanks,
> /Erik
> 
> On 2018-06-05 17:01, Roman Kennke wrote:
>> Am 05.06.2018 um 17:00 schrieb Erik Österlund:
>>> Hi Roman,
>>>
>>> On 2018-06-05 16:16, Roman Kennke wrote:
>>>> Am 04.06.2018 um 14:38 schrieb Erik Österlund:
>>>>> Hi Roman,
>>>>>
>>>>>    42
>>>>>    43   virtual void obj_equals(MacroAssembler* masm, DecoratorSet
>>>>> decorators,
>>>>>    44                           Register obj1, Register obj2);
>>>>>    45
>>>>>
>>>>> I don't think we need to pass in any decorators here. Perhaps one day
>>>>> there will be some important semantic property to deal with, but
>>>>> today I
>>>>> do not think there are any properties we care about, except possibly
>>>>> AS_RAW, but that would never propagate into the BarrierSetAssembler
>>>>> anyway.
>>>>>
>>>>> On that topic, I noticed that today we do the raw version of e.g.
>>>>> load_heap_oop inside of the BarrierSetAssembler, and to use it you
>>>>> would
>>>>> call load_heap_oop(AS_RAW). But the cmpoop stuff does it in a
>>>>> different
>>>>> way (cmpoop_raw in the macro assembler). I think it would be ideal
>>>>> if we
>>>>> could do it the same way, which would involve calling cmpoop with
>>>>> AS_RAW
>>>>> to get a raw oop comparison, residing in BarrierSetAssembler, with the
>>>>> usual hardwiring in the corresponding macro assembler function when it
>>>>> observes AS_RAW.
>>>>>
>>>>> So it would look something like this:
>>>>>
>>>>> void cmpoop(Register src1, Address src2, DecoratorSet decorators =
>>>>> AS_NORMAL);
>>>>>
>>>>> What do you think?
>>>> cmpoop_raw() is not the AS_RAW base implementation. It's only there to
>>>> help BarrierSetAssembler to implement the base
>>>> obj_equals(Address|Register, jobject). We cannot access cmp_literal32()
>>>> from outside the MacroAssembler.
>>> In other words, there is no AS_RAW option "exposed" to public use,
>>> right? Maybe there is no need for raw equals in our assembly code.
>> Yes. That is correct.
>>
>>>> The mentioned hardwiring to call straight to BSA is probably going
>>>> away too:
>>>> https://bugs.openjdk.java.net/browse/JDK-8203232
>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-May/032240.html
>>> I'm not sure I'm convinced that is an improvement. The expected
>>> behaviour at the callsite is that the code in BarrierSetAssembler (which
>>> is the level of the hierarchy that implements raw accesses) is run, and
>>> nothing else. If anything else happens, it's a bug. So I hardwire that
>>> at the callsite to always match the expected behaviour. To instead let
>>> each level of the barrier class hierarchy remember to check for AS_RAW
>>> and delegate to the parent class in a way that ultimately has the exact
>>> same perceivable effect as the hardwiring, but in a much more error
>>> prone way, does not sound like an improvement to me. Perhaps I can be
>>> convinced otherwise if I understand what the concern is here and what
>>> problem we are trying to solve.
>> I don't lean very much either way. But it should be discussed under
>> JDK-8203232. Considering that there is no use for cmpoop_raw() except as
>> helper for BSA, do you agree that we don't need the AS_RAW hardwiring
>> for obj_equals() ? Can I consider this patch Reviewed?
>>
>> Thanks, Roman
>>
>