Request for reviews (S): 6959430: Make sure raw loads have control edge

[Vladimir Kozlov]

Thank you, Tom

Vladimir

Tom Rodriguez wrote:
> That looks ok.
> 
> tom
> 
> On Jun 9, 2010, at 4:11 PM, Vladimir Kozlov wrote:
> 
>> I updated changes:
>>
>> http://cr.openjdk.java.net/~kvn/6959430/webrev.01
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> On Jun 9, 2010, at 12:07 PM, Vladimir Kozlov wrote:
>>>> Tom Rodriguez wrote:
>>>>> On Jun 9, 2010, at 11:04 AM, Vladimir Kozlov wrote:
>>>>>> Tom Rodriguez wrote:
>>>>>>> Looking at the webrev shows the problem with requiring control on every raw load.  These loads:
>>>>>>> !   Node* threadObj = make_load(NULL, p, thread_type, T_OBJECT);
>>>>>>> !   Node* threadObj = make_load(control(), p, thread_type, T_OBJECT);
>>>>>> I did it because threadObj could be moved during GC/safepoint (thread.cpp):
>>>>>>
>>>>>> // Traverse instance variables at the end since the GC may be moving things
>>>>>> // around using this function
>>>>>> f->do_oop((oop*) &_threadObj);
>>>>> The load is treated as an oop so that doesn't matter.  If it moves then it will be updated in the frame.
>>>> You are right.
>>>>
>>>>>>> Why did you change the memory here?
>>>>>>> !   Node* n3 = new(C, 3) LoadINode(NULL, immutable_memory(), p3, TypeRawPtr::BOTTOM);
>>>>>>> !   Node* n3 = new(C, 3) LoadINode(NULL, memory(p3), p3, _gvn.type(p3)->is_ptr());
>>>>>> I copied code from GraphKit::gen_subtype_check() to avoid using RAW ptr.
>>>>> Raw is definitely wrong for this since KlassPtr+super_check_offset has it's own alias category.  If the other uses memory(p) instead of immutable that's fine though I think they both could use immutable.  Whether it would matter to performance is hard to tell.
>>>> I found another case in LibraryCallKit::inline_native_hashcode():
>>> Those both probably should have control though I think that any failure would just result in the going slow path because of the way that those values change.
>>> tom
>>>> @@ -3512,8 +3512,7 @@ bool LibraryCallKit::inline_native_hashc
>>>>
>>>>  // Get the header out of the object, use LoadMarkNode when available
>>>>  Node* header_addr = basic_plus_adr(obj, oopDesc::mark_offset_in_bytes());
>>>> -  Node* header = make_load(NULL, header_addr, TypeRawPtr::BOTTOM, T_ADDRESS);
>>>> -  header = _gvn.transform( new (C, 2) CastP2XNode(NULL, header) );
>>>> +  Node* header = make_load(control(), header_addr, TypeX_X, TypeX_X->basic_type());
>>>>
>>>>  // Test the header to see if it is unlocked.
>>>>  Node *lock_mask      = _gvn.MakeConX(markOopDesc::biased_lock_mask_in_place);
>>>>
>>>> And I am worried about next load in initialize_object(), prototype_header can change value during safepoint:
>>>>
>>>> // For now only enable fast locking for non-array types
>>>> if (UseBiasedLocking && (length == NULL)) {
>>>>   mark_node = make_load(NULL, rawmem, klass_node, Klass::prototype_header_offset_in_bytes() + sizeof(oopDesc), TypeRawPtr::BOTTOM, T_ADDRESS);
>>>> } else {
>>>>   mark_node = makecon(TypeRawPtr::make((address)markOopDesc::prototype()));
>>>> }
>>>> rawmem = make_store(control, rawmem, object, oopDesc::mark_offset_in_bytes(), mark_node, T_ADDRESS);
>>>>
>>>> Vladimir
>>>>
>>>>> tom
>>>>>> Vladimir
>>>>>>
>>>>>>> tom
>>>>>>> On Jun 8, 2010, at 7:12 PM, Vladimir Kozlov wrote:
>>>>>>>> I will push it after 6953058 fix is pulled into our repository.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~kvn/6959430/webrev
>>>>>>>>
>>>>>>>> Fixed 6959430: Make sure raw loads have control edge
>>>>>>>>
>>>>>>>> Safepoint polls don't produce a new raw memory state
>>>>>>>> so loads of raw are allowed to float above safepoint
>>>>>>>> during which a raw value could be modified.
>>>>>>>>
>>>>>>>> Note, I added two checks. One is in factory methods
>>>>>>>> so the call stack will show where it is called from.
>>>>>>>> An other is in final graph reshape code to catch
>>>>>>>> nodes for which constructors were used directly.
>>>>>>>>
>>>>>>>> Passed CTW and JPRT.
>