[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR(S): 8069191: moving predicate out of loops may cause array accesses to bypass null check
From:       Roland Westrelin <roland.westrelin () oracle ! com>
Date:       2015-03-24 12:55:02
Message-ID: F8959E8E-ECA1-42DD-BC8A-AA7CD750F5C2 () oracle ! com
[Download RAW message or body]

See inlined.

> > Thanks for looking at this.
> > 
> > > This is what I waited for long time! Thank you for doing this, Roland.
> > > 
> > > How you handle case when CastPP is input of Phi node?
> > 
> > The Phi has a control so any memory node that depends on the Phi output is \
> > guaranteed to be "after" the CastPP, right?
> 
> Yes. So you simple remove CastPP in such case. Okay.
> 
> > 
> > > I am worried how you separate cases when precedence edge added from CastPP and \
> > > other precedence cases. Can you explain? May be there are no problems.
> > 
> > The
> > 
> > if (m->is_block_proj()) {
> > 
> > test guarantees that the precedence edge is a control node. And I assume it's \
> > always ok to remove the precedence edge and adjust the control when the \
> > precedence edge is a control node. Do you think that could break something?
> 
> Only if control edge came from CastPP. I know it is additional work but can you run \
> something (CTW? jvm98) and look what types of precedence edges GCM can see? \
> Unfortunately I don't remember what we have there. There are a lot of places where \
> we use add_prec(), mostly add pointers to memory nodes. If control nodes come only \
> from CastPP then I am fine with your code.

I added debugging code (that I didn't keep in the webrev below) that added (memory \
operation, control from CastPP) pairs in a side table during final graph reshaping, \
updated the pairs during matching and checked that all nodes that gcm sees with a \
control precedence got it from a CastPP. I ran CTW and other tests with that code and \
all tests passed. During that testing, I noticed that:

- CastPP nodes don't always have a control
- some CastPP nodes depend on a Region because the test was moved to the branch of a \
                dominating If
- the test for some CastPP's nodes are removed during escape analysis

I updated the code to reflect those cases.

http://cr.openjdk.java.net/~roland/8069191/webrev.01/

Roland.

> 
> Thanks,
> Vladimir
> 
> > 
> > Roland.
> > 
> > > 
> > > Thanks,
> > > Vladimir
> > > 
> > > On 3/17/15 3:54 AM, Roland Westrelin wrote:
> > > > http://cr.openjdk.java.net/~roland/8069191/webrev.00/
> > > > 
> > > > In the test (that needs to be run with StressGCM to cause incorrect code \
> > > > generation), a dependency carried by a CastPP is lost when CastPPs are \
> > > > removed after CCP. Detailed description of the bug is in: 
> > > > https://bugs.openjdk.java.net/browse/JDK-8069191
> > > > 
> > > > Vladimir suggested investigating the performance impact of keeping the \
> > > > CastPPs for the entire compilation. I found that this still causes \
> > > > performance regressions as documented in: 
> > > > https://bugs.openjdk.java.net/browse/JDK-8039999
> > > > 
> > > > The fix I suggest is to keep CastPPs during optimizations and remove then \
> > > > during final graph reshaping. To not loose the dependencies they carry, \
> > > > precedence edges are added to memory operations that depend on them. During \
> > > > GCM, the control of the memory operations to take the current control and the \
> > > > precedence edges. 
> > > > Experiments show that this scheme doesn't cause performance regressions (I \
> > > > ran promotion testing on x64 and sparc). 
> > > > Roland.
> > > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]