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

[Vladimir Kozlov]

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