RFR(S): 8078866: compiler/eliminateAutobox/6934604/TestIntBoxing.java assert(p_f->Opcode() == Op_IfFalse) failed

[Roland Westrelin]

Thanks for looking at this, Vladimir.

> Why do you think that main- and post- loops will be empty too and it is safe to replace them with pre-loop? Why it is safe?
> 
> Opaque1 node is used only when RCE and align is requsted:
>  cmp_end->set_req(2, peel_only ? pre_limit : pre_opaq);
> 
> In peel_only case pre-loop have only 1 iteration and can be eliminated too. The assert could be hit in such case too. So your change may not fix the problem.

In case of peel_only, we set:

if( peel_only ) main_head->set_main_no_pre_loop();

and in IdealLoopTree::policy_range_check():

  // If we unrolled with no intention of doing RCE and we later                                                                                                                                                                                                                 
  // changed our minds, we got no pre-loop.  Either we need to                                                                                                                                                                                                                   
  // make a new pre-loop, or we gotta disallow RCE.                                                                                                                                                                                                                             
  if (cl->is_main_no_pre_loop()) return false; // Disallowed for now.               

> I think eliminating main- and post- is good investigation for separate RFE. And we need to make sure it is safe.

My reasoning is that if the pre loop’s limit is still an opaque node, then the compiler has not been able to optimize the pre loop based on its limited number of iterations yet. I don’t see what optimization would remove stuff from the pre loop that wouldn’t have removed the same stuff from the initial loop, had it not been cloned. I could be missing something, though. Do you see one?

Roland.

> 
> To fix this bug, I think, we should bailout from do_range_check() when we can't find pre-loop.
> 
> Typo 'cab':
> +     // post loop cab be removed as well
> 
> Thanks,
> Vladimir
> 
> 
> On 5/13/15 6:02 AM, Roland Westrelin wrote:
>> http://cr.openjdk.java.net/~roland/8078866/webrev.00/
>> 
>> The assert fires when optimizing that code:
>> 
>>   static int remi_sumb() {
>>     Integer j = Integer.valueOf(1);
>>     for (int i = 0; i< 1000; i++) {
>>       j = j + 1;
>>     }
>>     return j;
>>   }
>> 
>> The compiler tries to find the pre loop from the main loop but fails because the pre loop isn’t there.
>> 
>> Here is the chain of events that leads to the pre loop being optimized out:
>> 
>> 1- The CountedLoopNode is created
>> 2- PhiNode::Value() for the induction variable’s Phi is called and capture that 0 <= i < 1000
>> 3- SplitIf is applied and moves the LoadI that reads the value from the Integer through the Phi that merges successive value of j
>> 4- pre-loop, main-loop, post-loop are created
>> 5- SplitIf is applied again and moves the LoadI of the loop through the Phi that merges both branches of:
>> 
>>     public static Integer valueOf(int i) {
>>         if (i >= IntegerCache.low && i <= IntegerCache.high)
>>             return IntegerCache.cache[i + (-IntegerCache.low)];
>>         return new Integer(i);
>>     }
>> 
>> 6- The LoadI is optimized out, the Phi for j is now a Phi that merges integer values, PhaseIdealLoop::replace_parallel_iv() replaces j by an expression that depends on i
>> 7- In the preloop, the range of values of i are known because of 2- so and the if of valueOf() can be optimized out
>> 8- the preloop is empty and is removed by IdealLoopTree::policy_do_remove_empty_loop()
>> 
>> SplitIf doesn’t find all opportunities for improvements before the pre/main/post loops are built. I considered trying to fix that but that seems complex and not robust enough: some optimizations after SplitIf could uncover more opportunities for SplitIf. The valueOf’s if cannot be optimized out in the main loop because the range of values of i in the main depends on the Phi for the iv of the pre loop and is not [0, 1000[ so I don’t see a way to make sure the compiler always optimizes the main loop out when the pre loop is empty.
>> 
>> Instead, I’m proposing that when IdealLoopTree::policy_do_remove_empty_loop() removes a loop, it checks whether it is a pre-loop and if it is, removes the main and post loops.
>> 
>> I don’t have a test case because it seems this only occurs under well defined conditions that are hard to reproduce with a simple test case: PhiNode::Value() must be called early, SplitIf must do a little work but not too much before pre/main/post loops are built. Actually, this occurs only when remi_sumb is inlined. If remi_sumb is not, we don’t optimize the loop out at all.
>> 
>> Roland.
>>