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

[Tom Rodriguez]

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