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

[Vladimir Kozlov]

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

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

Vladimir

> 
> tom
> 
>>
>>
>