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

[Vladimir Kozlov]

I don't remember I responded on this.

I think you should use lower_bound_proj as input for upper proj:

ProjNode* upper_bound_proj = create_new_if_for_predicate(predicate_proj);

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

You should call _igvn.hash_delete() when you change boolean input for new_predicate_iff, upper_bound_iff and lower_bound_iff.

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
>