RFR: 8203353: Fixup inferred decorators in the interpreter

[Erik Osterlund]

Hi Coleen,

Thanks for the review.

/Erik

> On 31 May 2018, at 21:23, coleen.phillimore at oracle.com wrote:
> 
> 
> 
>> On 5/31/18 3:01 PM, Roman Kennke wrote:
>>> Am 31.05.2018 um 20:13 schrieb coleen.phillimore at oracle.com:
>>> 
>>>> On 5/31/18 2:10 PM, Roman Kennke wrote:
>>>>> Am 31.05.2018 um 19:21 schrieb coleen.phillimore at Oracle.com:
>>>>> http://cr.openjdk.java.net/~eosterlund/8203353/webrev.00/src/hotspot/cpu/x86/macroAssembler_x86.cpp.frames.html
>>>>> 
>>>>> 
>>>>> 
>>>>> 6334 void MacroAssembler::access_store_at(BasicType type, DecoratorSet
>>>>> decorators, Address dst, Register src,
>>>>> 6335                                      Register tmp1, Register
>>>>> tmp2) {
>>>>> 6336   BarrierSetAssembler* bs =
>>>>> BarrierSet::barrier_set()->barrier_set_assembler();
>>>>> 6337 decorators = AccessInternal::decorator_fixup(decorators);
>>>>> 6338   bool as_raw = (decorators & AS_RAW) != 0;
>>>>> 6339   if (as_raw) {
>>>>> 6340     bs->BarrierSetAssembler::store_at(this, decorators, type, dst,
>>>>> src, tmp1, tmp2);
>>>>> 6341   } else {
>>>>> 6342     bs->store_at(this, decorators, type, dst, src, tmp1, tmp2);
>>>>> 6343   }
>>>>> 6344 }
>>>>> 
>>>>> This is a very strange function.  From the macroAssembler, how are we to
>>>>> know that the cases in this if statement are different?  It seems like
>>>>> this distinction should be made in BarrierSetAssembler class.   It also
>>>>> seems like whatever decorator fixup should also happen inside the
>>>>> BarrierSetAssembler, because the placement of these calls to fixup the
>>>>> decorators seems unknowable.
>>>> The if statement will go away with this change:
>>>> https://bugs.openjdk.java.net/browse/JDK-8203232
>>> Ok, thanks!  Should the fixup decorators adjustment be done in the
>>> callee then?
>> I think doing it in the access_X_at() helper functions is ok. It should
>> be done before calling into the BarrierSetAssembler methods (unless we
>> want to require each implementation to do the fixup, which would be
>> ugly), but not at each callee.
> 
> Ok.  I'm fine with the change then.
> 
> Thanks,
> Coleen
>> Roman
>> 
>> 
>>> thanks,
>>> Coleen
>>>> Roman
>>>> 
>>>>> thanks,
>>>>> Coleen
>>>>> 
>>>>>> On 5/31/18 10:23 AM, Erik Österlund wrote:
>>>>>> Hi Roman,
>>>>>> 
>>>>>> Thanks for the review!
>>>>>> 
>>>>>> /Erik
>>>>>> 
>>>>>> On 2018-05-31 15:40, Roman Kennke wrote:
>>>>>>>> Hi,
>>>>>>>> 
>>>>>>>> All subsystems that the access API touches, except the interpreter,
>>>>>>>> infers a set of decorators according to rules specified in
>>>>>>>> accessDecorators.hpp. These are typically sane defaults for things
>>>>>>>> like
>>>>>>>> reference strength, memory ordering, as well as inferring that
>>>>>>>> IN_HEAP_ARRAY is IN_HEAP, and IN_CONCURRENT_ROOT is IN_ROOT. The
>>>>>>>> interpreter should do that too.
>>>>>>>> 
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8203353/webrev.00/
>>>>>>>> 
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8203353
>>>>>>> Hi Erik, this seems reasonable and patch looks good. Thanks!
>>>>>>> Roman
>>>>>>> 
>> 
>