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

List:       openjdk-hotspot-compiler-dev
Subject:    Request for reviews (M): 7070134: Hotspot crashes with sigsegv from PorterStemmer
From:       vladimir.kozlov () oracle ! com (Vladimir Kozlov)
Date:       2011-07-27 0:10:50
Message-ID: 4E2F578A.2050508 () oracle ! com
[Download RAW message or body]

Thank you, Tom

Vladimir

Tom Rodriguez wrote:
> On Jul 26, 2011, at 4:49 PM, Vladimir Kozlov wrote:
> 
> > Thank you, Tom
> > 
> > Tom Rodriguez wrote:
> > > On Jul 26, 2011, at 1:09 PM, Vladimir Kozlov wrote:
> > > > http://cr.openjdk.java.net/~kvn/7070134/webrev
> > > > 
> > > > Fixed 7070134: Hotspot crashes with sigsegv from PorterStemmer
> > > > 
> > > > It is an other case of "6831314: C2 may incorrectly change control of type \
> > > > nodes". Loop predicate RCE upper_bound check matches an other dominating \
> > > > RangeCheck and split_if optimization moves loads to dominating test. In the \
> > > > bug case (step4() method) two code branches have the same loads from the same \
> > > > array (b[]) and they are combined and moved above array's index check and \
> > > > above lower_bound predicate check. 
> > > > The fix is band-aid to not move data nodes which are attached to a predicate \
> > > > test to a dominating test. It is allowed to do that during loop peeling and \
> > > > loop predicates generation since they duplicate all checks. I also switched \
> > > > off predicate RCE optimization for counted loops with '!=' test since there \
> > > > is no guarantee that loop index will be in the range [init, limit) if init > \
> > > > limit. 
> > > > Added regression test. Refwokload (x64) shows no affect.
> > > Why are the fixes in IfNode::dominated_by and PhaseIdealLoop::dominated_by \
> > > different?  IfNode just picks a different node but PhaseIdealLoop gives up.  \
> > > Can't PhaseIdealLoop pick a safe node?
> > PhaseIdealLoop::dominated_by does not give up. Data nodes control edge will be \
> > changed to iff->in(0) later during Ideal transformation when this If node folds \
> > (since Bool node was replaced with constant). If you want, I can do it explicitly \
> > in dominated_by(): replace prevdom with iff->in(0).
> 
> Oh I see.  I didn't follow it closely enough.  I think it's fine like it is.
> 
> > > loopPredicate.cpp:
> > > Is this line really needed?
> > > !       Node* upper_bound_bol = rc_predicate(loop, ctrl, scale, offset, init, \
> > >                 limit, stride, rng, true);
> > > !       Node* upper_bound_bol = rc_predicate(loop, lower_bound_proj, scale, \
> > > offset, init, limit, stride, rng, true); I'm not against it since it's valid \
> > > and maybe conceptually more correct but I'm unclear how it could have any \
> > > effect on correctness.
> > I changed it just for that reason: it is conceptually more correct. But I agree, \
> > it is not needed.
> 
> OK.  Look good.
> 
> tom
> 
> > Vladimir
> > 
> > > tom
> > > > 
> 


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