Request for reviews (M): 7070134: Hotspot crashes with sigsegv from PorterStemmer (original) (raw)

Tom Rodriguez tom.rodriguez at oracle.com
Tue Jul 26 16:59:09 PDT 2011

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 upperbound check matches an other dominating RangeCheck and splitif 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 lowerbound 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::dominatedby and PhaseIdealLoop::dominatedby different? IfNode just picks a different node but PhaseIdealLoop gives up. Can't PhaseIdealLoop pick a safe node? PhaseIdealLoop::dominatedby 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 dominatedby(): 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* upperboundbol = rcpredicate(loop, ctrl, scale, offset, init, limit, stride, rng, true); ! Node* upperboundbol = rcpredicate(loop, lowerboundproj, 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

More information about the hotspot-compiler-dev mailing list