Request for reviews (XXL): 5091921: Sign flip issues in loop optimizer

[Vladimir Kozlov]

Tom Rodriguez wrote:
> 
> Use "original" or "unadjusted" instead.  Not adjusted doesn't sound right.

Replaced with "original".

>>> macro.cpp:
>>> It's not really on the macro list is it?
>> It IS on macro list. What is a problem to have it on macro list?
>>
>>  LoopLimitNode( Compile* C, Node *init, Node *limit, Node *stride ) : Node(0,init,limit,stride) {
>>    // Put it on the Macro nodes list to optimize during macro nodes expansion.
>>    init_flags(Flag_is_macro);
>>    C->add_macro_node(this);
>>  }
> 
> I didn't notice that part.  It just doesn't seem very much like the other macros.  Why is it a macro?  It's not expanded by macro.cpp.  Is it just so that it's reprocessed by GVN at the end?  Why isn't it just expanded by macro.cpp then?  I'm a bit dubious of guards like this in the first place:

Yes, I just need to reprocess it after all loop opts. Originally it was not on 
macro list but it failed because nobody put it on igvn worklist at the end of 
loop opts. So the choice was to create an other list or used existing one. I 
choose existing list. Opaque nodes are on macro list for exactly the same 
reason: we want optimize them out after all loop opts.

May be we should just put all nodes on IGVN worklist after all loop opts and do 
optimization before macro nodes expansion. We can mark that phase specially and 
check for it as you said. But it is for next round of changes since it may 
affect generated code and requires additional testing.

Thanks,
Vladimir

> 
> +   // Delay following optimizations until all loop optimizations
> +   // done to keep Ideal graph simple.
> +   if (!can_reshape || phase->C->major_progress())
> +     return NULL;
> 
> It seems more like there should be a more general way to ask what phase we are in instead.  Anyway, I'm fine if you leave it as is.  It just seems odd.  Maybe macro is the wrong name for what that list has become.
> 
> tom
> 
>> Thank you,
>> Vladimir
>>
>>> tom
>>> On Apr 28, 2011, at 4:04 PM, Vladimir Kozlov wrote:
>>>> http://cr.openjdk.java.net/~kvn/5091921/webrev
>>>>
>>>> Fixed 5091921: Sign flip issues in loop optimizer
>>>>
>>>> Loop optimizer in Server VM does not take into account integer overflow when it generates code for loop limits calculation during counted loop construction, unrolling and range check elimination. As result generated code will behave incorrectly when an integer overflow happens.
>>>>
>>>> 1. Added limit check for Counted loops which, if failed, causes recompilation of the method without counted loop. It based on loop predication code we have already but we don't need to copy it after corresponding counted loop is created (this is why a new flag is passed to loop predicate copy methods).
>>>>
>>>> if (limit>= max_int-stride)
>>>> uncommon_trap
>>>> counted_loop init, limit, stride
>>>>
>>>> 2. Long arithmetic is used to calculate exact limit only when it is needed: empty loop elimination and predicated range check elimination. New ideal macro node LoopLimitNode is used but corresponding mach node is created only for 32-bit x86 to get better assembler code. Sparc and x64 have long registers so generated assembler is good enough without specialized mach node. Also delay LoopLimitNode transformation until all loop optimizations are done (by marking LoopLimitNode as macro node).
>>>>
>>>> 3. New limit after unroll calculated as:
>>>>
>>>> new_limit = limit-stride
>>>> new_limit = (limit < new_limit) ? MININT : new_limit;
>>>>
>>>> instead of current expression:
>>>>
>>>> new_limit = init + (((limit-init)/stride)&(-2))*stride
>>>>
>>>> Added other checks to avoid limit value overflow during unroll. Also fully unroll loops with up to 3 trip count.
>>>>
>>>> 4. Added underflow check for normal compares in loops which are optimized out using range check elimination code (I also refactored the code):
>>>>
>>>> MIN_INT <= scale*I+offset < limit
>>>>
>>>> ----------------------------------------------
>>>> These changes are put under flags to debug associated problems. I also allowed to print inlining decisions in product VM since PrintInlining is diagnostic flag now. New regression tests are added based on associated bug reports.
>>>>
>>>> These changes cause performance regression in benchmarks. Some of regression cases can't not be avoided but some we will address in a future.
>>>>
>>>> And finally I want to thank java VM team from HP who provided nice test cases and even suggested fix. I used their idea for unroll limit fix.
>