RFR: 8031639: make dependency management (mostly) ci independent

[Roland Westrelin]

Doug,

Comments inlined below

> Thanks for the review. Responses inline below:
> 
> On Jan 20, 2014, at 5:13 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
> 
>> Hi Doug,
>> 
>>> webrev: http://cr.openjdk.java.net/~dnsimon/JDK-8031639/
>> 
>> More informative assert messages than “oops” would be appreciated.
> 
> Sorry, I’ll change the message to something more descriptive.
>> 
>> dependencies.hpp:
>> 
>> comment needs to be fixed:
>> 249   GrowableArray<int>*       _dep_seen;  // (seen[h->ident] & (1<<dept))
> 
> How would you like it changed (I don’t think I modified this from its original). Are you suggesting the comment needs to reflect its usage in both the original and the #ifdef GRAAL world?

I think h->ident is a reference to _ident from ciBaseObject which you don’t use anymore. A comment that states that you’re using some unique identifier as computed by DepValue would be clearer.

>> dependencies.cpp:
>> 
>> 75     if (is_java_primitive(elemt))  return;   // Ex:  int[][]
>> align comment with // Ex:  String[][]
> 
> Will do.
> 
>> 121 void Dependencies::assert_call_site_target_value(jobject call_site, jobject method_handle) {
>> 122   Klass* ctxk = JNIHandles::resolve(call_site)->klass();
>> 123   check_ctxk(ctxk);
>> 124   assert_common_2(call_site_target_value, DepValue(_oop_recorder, call_site), DepValue(_oop_recorder, method_handle));
>> 125 }
>> 
>> Don’t you need a VM_ENTRY?
> 
> Actually, the signature and implementation in this patch is a little of our date with respect to the current Graal code base. It’s now:
> 
>  void Dependencies::assert_call_site_target_value(oop call_site, oop method_handle) {
>    assert_common_2(call_site_target_value, DepValue(_oop_recorder, JNIHandles::make_local(call_site)), DepValue(_oop_recorder, JNIHandles::make_local(method_handle)));
>  }
> 
> This is only called from a point within a VM_ENTRY so I don’t think it needs another VM_ENTRY marker, right? Is there someway to assert whether code is within a VM_ENTRY?

ASSERT_IN_VM is what you would use, I think.

I don’t see the VM_ENTRY when it’s called from GraphBuilder::access_field() for instance.

> 
>> Have you verified that compilation time is not affected?
> 
> No. Can you please suggest a reliable way to do this? I’m assuming something like CompileTheWorld?

I was not suggesting a thorough experiment. Running your favorite benchmark with -XX:+CITime and checking nothing surprising happens is what I had in mind.

>> I don’t really feel comfortable with this change. I wonder if it wouldn’t be simpler to have a base class for Dependencies, then one subclass for Dependencies that work on ci objects and one subclass that work directly on objects and Metadata. Then either accept code duplication or maybe use a parallel hierarchy for DepValue.
> 
> If the compilation time is not impacted, wouldn’t it be better to avoid code duplication altogether?

To me, this change doesn’t fit well with existing abstractions. I don’t think ciObject::constant_encoding() exists so that we can call JNIHandles::resolve() on the result and get a naked oop. All metadata references are still embedded in ciMetadata and even though it’s safe to grab a direct pointer to a Metadata from the compilers with the current implementation of the meta data, we don’t do it. That’s why I’m not comfortable with this change. I don’t have a nice solution to suggest either.

Roland.

> 
> I’ll post another webrev after waiting a bit for further feedback.
> 
> -Doug