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

[Tom Rodriguez]

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
>>> 
>>>