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

[Tom Rodriguez]

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.