RFR: 8203353: Fixup inferred decorators in the interpreter

[Roman Kennke]

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.

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