[9] RFR(S): 8140574: C2 must re-execute checks after deoptimizing from merged uncommon traps

[Roland Westrelin]

> Second, you check only successors blocks which could be optimized out. ciTypeFlow has rpo list and _methodBlocks->block_containing(bci). May be we can do more general domination search if it is not expensive.

Is the check that Tobias proposes even sufficient for correctness?

if (i < 0) {
  if (some_other_test) {
  }
}
if (i >= length) {
}

The some_other_test test is a predecessor of the i >= length test.

I also think we need a true domination test.

Roland.

> 
> Thanks,
> Vladimir
> 
> On 10/29/15 10:14 PM, Tobias Hartmann wrote:
>> Hi Roland,
>> 
>>> I suppose if we can prove that the bci of the unc for the dominating if dominates the bci of the unc of the dominated if then we’re good. Maybe by iterating over the ciTypeFlow blocks of the method.
>> 
>> I implemented such a check using ciTypeFlow:
>> 
>> http://cr.openjdk.java.net/~thartmann/8140574/webrev.02/
>> 
>> It works fine for my test and I also verified that the range checks in TestExplicitRangeChecks are folded. The only problem is that the checks in 'test19' [1] are not folded anymore because they were inlined from 'test19_helper1' and 'test19_helper2'. The current approach only looks at the current method and does not check inlined methods. I think this could be done as well but would be much more complicated. Maybe we can do this in a follow up RFE.
>> 
>> What do you think?
>> 
>> Thanks,
>> Tobias
>> 
>> [1] http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/e18469511c58/test/compiler/rangechecks/TestExplicitRangeChecks.java#l362
>> 
>> 
>> On 28.10.2015 17:51, Tobias Hartmann wrote:
>>> Hi Roland,
>>> 
>>> thanks for the suggestions - I was able to create a regression test that triggers the problem you described:
>>> http://cr.openjdk.java.net/~thartmann/8140574/webrev.01/
>>> 
>>> I'll look into how we can fix this.
>>> 
>>> Best,
>>> Tobias
>>> 
>>> On 28.10.2015 14:33, Roland Westrelin wrote:
>>>>>> if (i < 0) {
>>>>>>  if (some_condition) {
>>>>>>    // never taken so unc unstable_if
>>>>>>  }
>>>>>> }
>>>>> 
>>>>> I agree that this would be a problem but why would be add an uncommon trap here? Either the i<0 branch is never executed, then we would add the uncommon trap like this
>>>>> 
>>>>> if (i < 0) {
>>>>>   // unc unstable_if
>>>>> }
>>>>> 
>>>>> or it was executed and then also "some_condition" was true and we would not add an uncommon trap at all.
>>>> 
>>>> Profiling is what tells us that we can cut one branch. It’s not 100% accurate.
>>>> 
>>>> We could have:
>>>> 
>>>> void m1(boolean flag) {
>>>>   if (i < 0) {
>>>>     if (flag) {
>>>>     }
>>>>   }
>>>> }
>>>> 
>>>> that is called a lot with flag false and i sometimes negative, sometimes positive so m1 is compiled, no more profiling is recorded and the profiling tells us the branch is never taken. Then we run:
>>>> 
>>>> void m2(int i) {
>>>>   ..
>>>>   m1(i, flag);
>>>> }
>>>> 
>>>> with i always positive. During parsing the compiler is unable to prove flag always true, so we add the unc and after some optimization, flag is proven always true so the test is removed.
>>>> 
>>>> For instance, this:
>>>> 
>>>> for (int i = 0; i < 10; i++);
>>>> flag = (i == 10);
>>>> 
>>>> I think it would be worthwhile to check we can write a test case like this.
>>>> 
>>>>>> So I think we have to restrict when we apply that transformation even more, maybe matching the expected bytecode pattern more closely?
>>>>> 
>>>>> I'm not sure how to do this. We would somehow have to make sure that the dominating uncommon trap "belongs" to the dominating if and not to some other if that was removed, right?
>>>> 
>>>> I suppose if we can prove that the bci of the unc for the dominating if dominates the bci of the unc of the dominated if then we’re good. Maybe by iterating over the ciTypeFlow blocks of the method.
>>>> 
>>>> Roland.
>>>>