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

Vladimir Kozlov vladimir.kozlov at
Wed Jun 9 12:07:46 PDT 2010

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():

@@ -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);


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

More information about the hotspot-compiler-dev mailing list