RFR: JDK-8203157: Object equals abstraction for BarrierSetAssembler

[Andrew Haley]

On 06/12/2018 10:23 AM, Roman Kennke wrote:
> Am 12.06.2018 um 11:11 schrieb Andrew Haley:
>> On 06/11/2018 08:17 PM, Roman Kennke wrote:
>>> Am 11.06.2018 um 19:11 schrieb Andrew Haley:
>>>> On 06/11/2018 04:56 PM, Andrew Haley wrote:
>>>>> On 06/08/2018 09:17 PM, Roman Kennke wrote:
>>>>>> Why is it better? And how would I do that? It sounds like a fairly
>>>>>> complex undertaking for a special case. Notice that if the oop doesn't
>>>>>> qualify as immediate operand (quite likely for an oop?) it used to be
>>>>>> moved into rscratch1 anyway a few lines below.
>>>>>
>>>>> Sorry for the slow reply.  I'm looking now.
>>>>
>>>> OK.  The problem is that this is a very bad code smell:
>>>>
>>>>       case T_ARRAY:
>>>>         jobject2reg(opr2->as_constant_ptr()->as_jobject(), rscratch1);
>>>>         __ cmpoop(reg1, rscratch1);
>>>>
>>>> I can't tell that this is correct.  rscratch1 is used by assembler
>>>> macros, and I don't know if some other GC (e.g. ZGC) might need to use
>>>> rscratch1 inside cmpoop.  The risk here is obvious.  The Right Thing
>>>> to do IMO is to generate a scratch register for pointer comparisons.
>>>>
>>>> Unless, I guess, we know that cmpoop never ever needs a scratch
>>>> register for any forseeable garbage collector.
>>>>
>>>
>>> I do know that Shenandoah does not require a tmp reg. I also do know
>>> that no other collector currently needs equals-barriers at all.
>>
>> So cmpoop() is literally useless.  It does nothing except add a layer
>> of obfuscation in the name of some possible future collector.
> 
> The layer of abstraction is needed by Shenandoah. We need special
> handling for comparing oops. It is certainly not useless. Or are we
> talking about different issues?

Ah, okay.  I'm looking at ShenandoahBarrierSetAssembler::obj_equals()
and I see that it actually has a side effect on its operands rather
than using scratch registers.  Ewww.  I get it now.

OK, I withdraw my objection.  It's very confusing code to read, but
it is what it is.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671