Fix bug with CHA dependencies by recording a set of classes for registering code.

Fix bug with CHA dependencies by recording a set of classes for registering code.

CHA is not only used with the receiver class, but also with other classes
when doing type propagation. The optimized code has to be registered with
all classes that are affected by CHA so that deoptimization occurs whenever
the set of subclasses changes for these classes.

R=vegorov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39194

[Florian Schneider, 2014-08-12 11:37:10]

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.h
File runtime/vm/cha.h (left):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.h#oldcode29
runtime/vm/cha.h:29: static ZoneGrowableArray<intptr_t>*
GetSubclassIdsOf(intptr_t cid);
I removed these methods since are never used (except in tests).

https://codereview.chromium.org/463103002/diff/1/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/compiler.cc#newco...
runtime/vm/compiler.cc:579:
isolate->cha()->cha_classes()[i]->RegisterCHACode(code);
This fixes bug #2: The optimized code is now registered with a set of classes
recorded in CHA during compilation.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:526: if (FLAG_use_cha &&
IsKnownPrivateClass(type_class)) {
This is the fix for bug #1.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:792:
CompileType::FromAbstractType(dst_type(), value_type->is_nullable()));
This line and the change below add tracking of nullability through
AssertAssignable which was missing before.

[Vyacheslav Egorov (Google), 2014-08-12 12:16:56]

first round of comments, I will wait for final revision that removes
IsKnownPrivateClass and handles is_implemented as part of CHA.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc#newcode23
runtime/vm/cha.cc:23: 
add empty line

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc#newcode35
runtime/vm/cha.cc:35: bool CHA::HasSubclassesSafe(intptr_t cid) {
I find the name HasSubclassesSafe confusing.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc#newcode61
runtime/vm/cha.cc:61: AddToCHAClasses(&Class::ZoneHandle(cls.raw()));
Maybe it's better to have a single return false in the function?

if (!cls_direct_subclasses.IsNull()) {
  // loop goes here
}

AddToChaClasses(...);
return false;

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha_test.cc
File runtime/vm/cha_test.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha_test.cc#newco...
runtime/vm/cha_test.cc:89: EXPECT(!cha.HasSubclasses(class_d_id));
You should also test if list of classes were collected correctly.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:3902: const Class& type_class =
Class::ZoneHandle(type.type_class());
Why ZoneHandle?

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:526: if (FLAG_use_cha &&
IsKnownPrivateClass(type_class)) {
On 2014/08/12 11:37:10, Florian Schneider wrote:
> This is the fix for bug #1.

Something is not right here. I suggest we just kill IsKnownPrivateClass(), I
don't think it is necessary anymore.

[srdjan, 2014-08-12 17:29:53]

DBC

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc#newcode15
runtime/vm/cha.cc:15: void CHA::AddToCHAClasses(Class* cls) {
This would be clearer if renamed to AddToLeafClasses (or similar). A "CHA class"
is not really self describing name. Same goes for instance field cha_classes_.

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc#newcode25
runtime/vm/cha.cc:25: bool CHA::HasSubclasses(const Class& cls) {
Maybe add a comment in the .h file that this updates the list of leaf classes if
necessary.

[Florian Schneider, 2014-08-13 10:34:41]

Addressed comments and another bug fixed bug with CHA and interface
implementation.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc#newcode23
runtime/vm/cha.cc:23: 
On 2014/08/12 12:16:55, Vyacheslav Egorov (Google wrote:
> add empty line

Done.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha.cc#newcode35
runtime/vm/cha.cc:35: bool CHA::HasSubclassesSafe(intptr_t cid) {
On 2014/08/12 12:16:55, Vyacheslav Egorov (Google wrote:
> I find the name HasSubclassesSafe confusing. 

I removed it.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha_test.cc
File runtime/vm/cha_test.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/cha_test.cc#newco...
runtime/vm/cha_test.cc:89: EXPECT(!cha.HasSubclasses(class_d_id));
On 2014/08/12 12:16:55, Vyacheslav Egorov (Google wrote:
> You should also test if list of classes were collected correctly.

Done.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_optimi...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_optimi...
runtime/vm/flow_graph_optimizer.cc:3902: const Class& type_class =
Class::ZoneHandle(type.type_class());
On 2014/08/12 12:16:56, Vyacheslav Egorov (Google wrote:
> Why ZoneHandle?

Done.

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/463103002/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:526: if (FLAG_use_cha &&
IsKnownPrivateClass(type_class)) {
On 2014/08/12 12:16:56, Vyacheslav Egorov (Google wrote:
> On 2014/08/12 11:37:10, Florian Schneider wrote:
> > This is the fix for bug #1.
> 
> Something is not right here. I suggest we just kill IsKnownPrivateClass(), I
> don't think it is necessary anymore.

Done. Also removed HasSubclassesSafe. There is only one version now. I also
added CHA::IsImplemented to track interface implementation properly.

[Vyacheslav Egorov (Google), 2014-08-13 11:40:28]

lgtm

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode15
runtime/vm/cha.cc:15: void CHA::AddToCHAClasses(Class* cls) {
Accept an normal Handle here?

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode21
runtime/vm/cha.cc:21: cha_classes_.Add(cls);
Rewrap into ZoneHandle here.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode26
runtime/vm/cha.cc:26: // or implementable by users).
The key point is that it can't be extended later after startup, not that it
can't be extended by users.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode27
runtime/vm/cha.cc:27: // (TODO): Allow more libraries.
TODO(fschneider) ? :)

strange TODO syntax.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode57
runtime/vm/cha.cc:57: AddToCHAClasses(&Class::ZoneHandle(cls.raw()));
I think you should move IsKnownPrivateClass into the AddToCHAClasses and just do
an early return from there.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:103: ifc ^= type.type_class();
I wonder if you need to skip Malformed types here?

Superclass one does !super_type.IsMalformed() &&
super_type.HasResolvedTypeClass() check here.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:294: 
Accidental edit?

[Florian Schneider, 2014-08-13 12:08:58]

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc#newcode15
runtime/vm/cha.cc:15: void CHA::AddToCHAClasses(Class* cls) {
On 2014/08/12 17:29:53, srdjan wrote:
> This would be clearer if renamed to AddToLeafClasses (or similar). A "CHA
class"
> is not really self describing name. Same goes for instance field cha_classes_.

Done.

https://codereview.chromium.org/463103002/diff/20001/runtime/vm/cha.cc#newcode25
runtime/vm/cha.cc:25: bool CHA::HasSubclasses(const Class& cls) {
On 2014/08/12 17:29:53, srdjan wrote:
> Maybe add a comment in the .h file that this updates the list of leaf classes
if
> necessary.

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode15
runtime/vm/cha.cc:15: void CHA::AddToCHAClasses(Class* cls) {
On 2014/08/13 11:40:28, Vyacheslav Egorov (Google wrote:
> Accept an normal Handle here?

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode21
runtime/vm/cha.cc:21: cha_classes_.Add(cls);
On 2014/08/13 11:40:27, Vyacheslav Egorov (Google wrote:
> Rewrap into ZoneHandle here.

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode27
runtime/vm/cha.cc:27: // (TODO): Allow more libraries.
On 2014/08/13 11:40:27, Vyacheslav Egorov (Google wrote:
> TODO(fschneider) ? :)
> 
> strange TODO syntax.
> 

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/cha.cc#newcode57
runtime/vm/cha.cc:57: AddToCHAClasses(&Class::ZoneHandle(cls.raw()));
On 2014/08/13 11:40:28, Vyacheslav Egorov (Google wrote:
> I think you should move IsKnownPrivateClass into the AddToCHAClasses and just
do
> an early return from there.

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:103: ifc ^= type.type_class();
On 2014/08/13 11:40:28, Vyacheslav Egorov (Google wrote:
> I wonder if you need to skip Malformed types here?
> 
> Superclass one does !super_type.IsMalformed() &&
> super_type.HasResolvedTypeClass() check here.

Done.

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/463103002/diff/40001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:294: 
On 2014/08/13 11:40:28, Vyacheslav Egorov (Google wrote:
> Accidental edit?

Done.

[Florian Schneider, 2014-08-13 12:43:18]

Committed patchset #4 manually as r39194 (presubmit successful).