Request for reviews (S): 6959430: Make sure raw loads have control edge
tom.rodriguez at oracle.com
Wed Jun 9 11:23:36 PDT 2010
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.
>> ! Node* osthread = make_load(NULL, p, TypeRawPtr::NOTNULL, T_ADDRESS);
>> ! Node* osthread = make_load(control(), p, TypeRawPtr::NOTNULL, T_ADDRESS);
> I agree that this may not need control.
>> will no longer common with their equivalents since they will all have control. Those loads will never need control because their value cannot change. Actually it would work better if they were using immutable_memory as well. We really need to split out the raw alias category into more classes. Maybe we need to allow no control in a few cases? We could capture it in a helper somewhere.
>> 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.
>> 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