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

[Vladimir Kozlov]

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