RFR: 8203353: Fixup inferred decorators in the interpreter

[coleen.phillimore at oracle.com]

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