review (S) for 6930043: C2: SIGSEGV in javasoft.sqe.tests.lang.arr017.arr01702.arr01702.loop_forw(II)I

[Vladimir Kozlov]

Looks good.

Vladimir

Tom Rodriguez wrote:
> It's updated now.
> 
> tom
> 
> On Mar 16, 2010, at 1:59 PM, Vladimir Kozlov wrote:
> 
>> Yes, I was wrong, it is inserted in between. The assert is good.
>> But I don't see updated comment and hash_delete() calls.
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> On Mar 16, 2010, at 11:17 AM, Vladimir Kozlov wrote:
>>>> Tom Rodriguez wrote:
>>>>> On Mar 16, 2010, at 10:46 AM, Vladimir Kozlov wrote:
>>>>>> I think you should use lower_bound_proj as input for upper proj:
>>>>>> ProjNode* upper_bound_proj = create_new_if_for_predicate(predicate_proj);
>>>>> Why?  That's not the usage model of create_new_if_for_predicate.
>>>> Then you have to swap next lines, otherwise upper_bound will be generated above lower_bound
>>>>
>>>> 2267       ProjNode* lower_bound_proj = create_new_if_for_predicate(predicate_proj);
>>>> 2268       ProjNode* upper_bound_proj = create_new_if_for_predicate(predicate_proj);
>>> create_new_if_for_predicate inserts a new if between the trap place holder if and the last created if.  I'd added an assert that verified the ordering but I guess I never updated the webrev itself.  It's there now and it looks like this:
>>> +       assert(upper_bound_proj->in(0)->as_If()->in(0) == lower_bound_proj, "should dominate");
>>> tom
>>>> Vladimir
>>>>
>>>>> ProjNode* PhaseIdealLoop::create_new_if_for_predicate(ProjNode* cont_proj) {
>>>>> assert(is_uncommon_trap_if_pattern(cont_proj, true), "must be a uct if pattern!");
>>>>> I think it would assert if I passed something else and it wouldn't improve the code shape even if I did.
>>>>>> Can you add to the next comment what you said in mail "late schedule may move invariant things inside the loop body"?
>>>>>> To remember why we need this clone.
>>>>>>
>>>>>> // Perform cloning to keep Invariance state correct
>>>>> ok.
>>>>>> You should call _igvn.hash_delete() when you change boolean input for new_predicate_iff, upper_bound_iff and lower_bound_iff.
>>>>> Calling hash_delete implies something which isn't true about the nodes since they are newly created and I found it confusing when I first read it which is why I removed it.  I guess it would be more consistent with the other code in loop opts which pretty much always calls hash_delete.  I'll restore it.
>>>>> tom
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> Tom Rodriguez wrote:
>>>>>>> It can have side effects because of the order that PhaseIdealLoop schedules things.  It performs a late schedule so invariant things can end up inside the loop body if they are only used in the loop.  The makes identifying loop members simpler but the Invariance helper class is used by the predication code to make clones outside the loop so that truly invariant values will appear invariant.  So I've rearranged the code a bit more so we any values we clone are create with the control of the higher if.  I've also redistributed some code which was shared before since I think it obfuscated what was really happening, particular once we needed to have two new ifs in the range check case.  I'm running CTW to make sure that some of the asserts I added don't fail but it works the same as before.  I've updated the webrev.
>>>>>>> tom
>>>>>>> On Mar 12, 2010, at 4:34 PM, Vladimir Kozlov wrote:
>>>>>>>> Fine. But as we discussed you need to verify that invar.clone() does not have any side effects.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>> That's what I get for doing edits just before generating the webrev.  I'd originally deleted the clone since it seems useless.  If it's invariant then cloning it doesn't change anything.  I chickened out at the last minuted and restored but didn't recompile.  I think I'll stick with deleting it.  I've regenerated the webrev.
>>>>>>>>> tom
>>>>>>>>> On Mar 12, 2010, at 3:05 PM, Vladimir Kozlov wrote:
>>>>>>>>>> Tom,
>>>>>>>>>>
>>>>>>>>>> You removed lines 2225,2226 but where ctrl node comes from for next line?:
>>>>>>>>>>
>>>>>>>>>> 2243       ld_rng = (LoadRangeNode*)invar.clone(ld_rng, ctrl);
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Vladimir
>>>>>>>>>>
>>>>>>>>>> Tom Rodriguez wrote:
>>>>>>>>>>> http://cr.openjdk.java.net/~never/6930043
>>>>>>>>>>> 6930043: C2: SIGSEGV in javasoft.sqe.tests.lang.arr017.arr01702.arr01702.loop_forw(II)I
>>>>>>>>>>> Reviewed-by:
>>>>>>>>>>> The new loop predication code is missing logic to test that the
>>>>>>>>>>> initial value of the index is in range.  In many cases will be
>>>>>>>>>>> eliminated statically.  Tested with failing test.  Also tested that
>>>>>>>>>>> this new test doesn't affect the performance improvement we were
>>>>>>>>>>> seeing with scimark.
>>>>>>>>>>> src/share/vm/opto/loopTransform.cpp
>>>>>>>>>>> test/compiler/6930043/Test6930043.java
>