Request for reviews (L): 6674588: Escape Analysis: Improve Escape Analysis code

[John Rose]

On Mar 13, 2008, at 2:42 PM, Vladimir Kozlov wrote:

> I updated changes and webrev.

That's some amazing work...

--- node.cpp:
+ (is_CheckCastPP() && req() == 2))

Omit the check of req(); that was just a speed hack to avoid calling  
the Opcode() virtual.

--- callnode.[ch]pp

+  // or result projection is there are several CheckCastPP
s/is there are/if there are/

+   return this;  // more than 1 CheckCastPP
s/return this/return p/ (the result projection), or else change the  
comment in the header file..

+ // The check above guaranty offset >= 0
s/guaranty/guarantees/.  Also, you could just make it an assert; it  
would be easier to read.

+  add_offset(-offset)
That makes me want TypePtr::cast_to_offset(0).  Maybe some day.

+  int base_idx = C->get_alias_index(adr_base_t);
+  int aat_idx = C->get_alias_index(aat->isa_oopptr());
+  if (base_idx == aat_idx ...

I don't think this works.  If you ask for the alias index of any oop  
type plus zero,
you will get the memory slice for *all* oopDesc::_mark fields.
So the above check should always be true.

Don't you want to get the alias index the offset versions of the  
pointers?
(I.e., do not add -offset to base, and *do* add offset to aat.)

--- escape.hpp

    The following note types are JavaObject
s/note/node/

+//    OF -P> JO (the object which oop is stored in the field)
s/which/whose/ (genitive neuter)

--- escape.cpp
+ adr_type == TypeRawPtr::NOTNULL

This check is overly specific; I think you can omit it, and it will  
make the code more robust.

+  // If we are still collecting or there were no not escaping  
allocations

Should be "there were no non-escaping allocations".  s/not esc/non-esc/g
There are several occurrences of "not esc", 2 in escape.cpp, and 2 in  
escape.hpp.


+  if (base->is_top()) { // The AddP case #3.
+    base = addp->in(AddPNode::Address)->uncast();

Should there be an assert here to correspond to your comment?
Something like base->is_Proj && base->in(0)->is_Allocate.

+          && igvn->type(alloc->in(AllocateNode::KlassNode)) !=  
TypeKlassPtr::OBJECT) {

This check seems like it will almost always be true, but if it is  
false it doesn't say much interesting, just that the compiler cannot  
make any conclusions about the type of the cloned object.

...I'm about 1/3 of the way through this file; I'll send comments for  
the rest of it later.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20080313/3313e207/attachment.html