RFR: 8203353: Fixup inferred decorators in the interpreter

[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?

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
>>>>
>