review (M) for 7024475: loop doesn't terminate when compiled

Tom Rodriguez tom.rodriguez at oracle.com
Fri Mar 25 16:38:46 PDT 2011


Thanks!

tom

On Mar 25, 2011, at 2:04 PM, Vladimir Kozlov wrote:

> It looks good.
> 
> Sorry, I somehow thought OSROnlyBCI is compile_id, if it is bci then -1 is fine. And I did not realized that you switched to old_arena() check, nice change.
> 
> Thanks,
> Vladimir
> 
> Tom Rodriguez wrote:
>> On Mar 25, 2011, at 12:53 PM, Vladimir Kozlov wrote:
>>> Add comment that guard is generated explicitly for main and post loops:
>>> 
>>> +   bool needs_guard = !cl->is_main_loop && !cl->is_post_loop();
>>> 
>> Ok.
>>> + if (bol->is_Bool() && bol->as_Bool()->_test._test == BoolTest::lt) {
>>> 
>>> should be:
>>> 
>>> + if (bol->is_Bool() && bol->as_Bool()->_test._test ==
>>> +                       cl->loopexit()->test_trip()) {
>> Ok.
>>> I will modify this code to skip cloned predicates for 7004535.
>>> 
>>> I think we need RFE to cleanup PrintOpto. I never used it since I thought it is duplicate of PrintCompilation.
>> Me either.  It always seemed like just a random collection of printing code.
>>> And thank you for fixing find()! I also saw that it spend a lot of time in old_arena()->contains(n) when you have big graph. And we need this check only after Matcher, during Optimize we have only one arena. It would be nice to avoid call to noninlined contains() with a simple check.
>> That's why I switched to old_arena instead of node_arena.  It starts out empty so the check is faster.  The inlining is only a minor difference I think though.
>>> Also we pass VectorSets as references but inside find_recur() we take pointer, why not just pass pointers?
>> I considered fixing that so I will go ahead.  I've updated the webrev with these changes.
>>> Why default value OSROnlyBCI is -1? I think it should be 0 since 0 should not be  used as compile_id. Otherwise it is confusing: based on the flag's comment it loops like you always exclude first OSR compilation.
>> It's a bci not a compile_id.  Given that I accept both negative and postive values there aren't any good values, though I guess max int would work.  That's why I check FLAG_IS_DEFAULT to determine whether to check the value.  I know it's kind of odd but I couldn't see a better way. Maybe it should operate like DeoptimizeOnlyAt instead:
>>        char buffer[8];
>>        jio_snprintf(buffer, sizeof(buffer), "%d", sd->bci());
>>        size_t len = strlen(buffer);
>>        const char * found = strstr(DeoptimizeOnlyAt, buffer);
>>        while (found != NULL) {
>>          if ((found[len] == ',' || found[len] == '\n' || found[len] == '\0') &&
>>              (found == DeoptimizeOnlyAt || found[-1] == ',' || found[-1] == '\n')) {
>>            // Check that the bci found is bracketed by terminators.
>>            break;
>>          }
>>          found = strstr(found + 1, buffer);
>>        }
>> That allows specifying multiple bcis.  I'd also considered adding an option directive so you could say something like:
>> -XX:CompileCommand=option,Test,test,OSROnlyBCI=49,600
>> to restrict it to a specific method too.
>> tom
>>> Thanks,
>>> Vladimir
>>> 
>>> Tom Rodriguez wrote:
>>>> http://cr.openjdk.java.net/~never/7024475
>>>> 7024475: loop doesn't terminate when compiled
>>>> Reviewed-by:
>>>> The code which replaces an empty loop with the final computed value
>>>> only works correctly if the the loop is guaranteed to execute once
>>>> since it replaces the conditionally executed loop body with a value.
>>>> Loop transformations like peeling generally result in zero trip guards
>>>> being generated but they aren't required to be there.  The fix is to
>>>> make sure we have a guard by peeling the loop first.  This results in
>>>> a small redundant test in some cases.  I added a check for obvious
>>>> ones, which occurred in scimark SOR.  Main and post loops are
>>>> constructed with zero trip guards so I assume one exists in those
>>>> cases.  Tested with new test case, original failing program,
>>>> refworkload and full CTW.
>>>> I also improved a few debugging features.  Node::find could get into
>>>> an infinite loop walking debug_orig, so it should be check for a
>>>> cycle.  I made some other efficiency improvements before I realized
>>>> that the problem was a cycle.  I went ahead and kept those changes.
>>>> IdealGraphPrinter should print out a few more debug fields and should
>>>> dump the block structure if it exists.  I changed the printing after
>>>> beautify loops so it only happens once instead of after each recursive
>>>> call.  I added an OSROnlyBCI flag to control which OSR bcis can be
>>>> compiled. I changed the assert in loop verification to allow verifying
>>>> when the igvn worklist isn't empty.



More information about the hotspot-compiler-dev mailing list