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

[Vladimir Kozlov]

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.

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

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.
>