Request for reviews (L): 6695810 and 6703890

[John Rose]

On May 19, 2008, at 9:31 PM, Vladimir Kozlov wrote:

> -----------------------------------------------
> http://webrev.invokedynamic.info/kvn/6695810_only/index.html
>
> Problem:
> After CastPP is removed by PhaseCCP EncodeP was moved above the  
> null check
> (EncodeP and DecodeN don't have control edge). As result the  
> NotNULL type
> is not valid any more.
>
> Solution:
> Set control edge for EncodeP and DecodeN nodes using  
> MemNode::Ideal_DU_PostCCP().
>
> Also fix next problems:
>  - replace Type::is_narrow() with more precise  
> Type::is_ptr_to_narrowoop()
>    defined during TypeOopPtr construction,
>  - use subclass check instead of alias types check in  
> CallNode::may_modify(),
>  - call PhiNode::split_out_instance() also for Phi nodes with
>    non-instance OopPtr type which matches the instance type,
>  - stop compilation instead of VM exit when there is no space
>    for scratch buffer in CodeCache,
>  - don't flatten instance type in alias types,
>  - skip the split through phi in LoadNode::Ideal() if the Region node
>    dominates load's address,
>  - an allocation is not scalar replaceable if the resuilt is stored
>    into unknown array's element,
>  - try 2 Region's input paths in Node::dominates(),
>  - add missing checks for ConN node.

--- type.hpp

The odd naming is distracting between TypePtr::points_to_narrowoop,  
TypePtr:: _is_ptr_to_narrowoop, Type::is_ptr_to_narrowoop.

Suggest s/points_to_narrowoop/is_ptr_to_narrowoop_nv/ (where_nv is  
already in use to mean "I'm a hacked non-virtual version".)

---memnode.cpp

Suggest a comment on MemNode::Ideal_DU_postCCP that n is not  
necessarily a memnode, and that nothing other than its control might  
be updated.

Suggest replacing the goto with structured control flow.  While you  
are at it, factor the "Split through Phi" stuff into a subroutine,  
since it is two screenfuls all by itself.

--- callnode.cpp

>  - use subclass check instead of alias types check in  
> CallNode::may_modify(),


The webrev patch differs from the diff.  Which is more current?  The  
patch looks more correct (and is a more conservative change).

Side comment:  This logic feels rickety to me.  (And the names are  
confusing.)  Why the pre-existing exemption of interfaces?  They act  
here just like Object.

The point is that an actual argument of type AT_K might be used to  
modify slice ADR_K+OFFSET, if AT_K and ADR_K overlap properly, and  
the type analysis determines that possibility.  This can happen if:
- AT_K and ADR_K are the same
- AT_K is an interface which ADR_K implements (actually, since  
interface types cannot be trusted in opto, AT_K is any interface)
- AT_K is a non-exact proper superclass of ADR_K (proposed fix  
disregards exactness of AT_K)

The diff (not the patch) includes this case:
- AT_K is a proper subclass of ADR_K

That doesn't follow, since ADR_K the exact type of an allocation site  
(not a super type), and if AT_K is a proper subclass, it can never  
refer to the same object.

Maybe a few more comments would help?

--- rest of files

The look good.  Reviewed.

-- John