RFR(S): 8146792: Predicate moved after partial peel may lead to broken graph

[Roland Westrelin]

> I am thinking that it is "always" safe to move pinned data node above original/dummy predicates (loop index variable is depending on limit check predicate, but we will never move index node from loop). We only needs to be sure that we move it (after partial peel, for example) before any dependent checks and data nodes are moved from the loop. Those checks and data will be inserted below it.

I get it now and I think you’re right but it would need to be done for all data nodes which sounds like a mess.

> Anyway, how rare this case? If it is vary rare I agree with your change since performance is not important.

It’s very rare. I’ve seen it only once running the old CTW with the castPP change from 8139771.

Roland.

> 
> Thanks,
> Vladimir
> 
>> 
>> Actually, I can reproduce this scenario with the patch below: some changes to the test and making range check smearing a little big more aggressive so a range check is replaced by a dominating predicate range check.
>> 
>> Roland.
>> 
>> diff --git a/src/share/vm/opto/ifnode.cpp b/src/share/vm/opto/ifnode.cpp
>> --- a/src/share/vm/opto/ifnode.cpp
>> +++ b/src/share/vm/opto/ifnode.cpp
>> @@ -514,7 +514,7 @@
>>    // along the OOB path.  Otherwise, it's possible that the user wrote
>>    // something which optimized to look like a range check but behaves
>>    // in some other way.
>> -  if (iftrap->is_uncommon_trap_proj(Deoptimization::Reason_range_check) == NULL) {
>> +  if (iftrap->is_uncommon_trap_proj(Deoptimization::Reason_none) == NULL) {
>>      return 0;
>>    }
>> 
>> diff --git a/test/compiler/loopopts/BadPredicateAfterPartialPeel.java b/test/compiler/loopopts/BadPredicateAfterPartialPeel.java
>> --- a/test/compiler/loopopts/BadPredicateAfterPartialPeel.java
>> +++ b/test/compiler/loopopts/BadPredicateAfterPartialPeel.java
>> @@ -30,6 +30,8 @@
>>   *
>>   */
>> 
>> +import java.util.Objects;
>> +
>>  public class BadPredicateAfterPartialPeel {
>> 
>>      static void not_inlined1() {}
>> @@ -45,13 +47,13 @@
>>      boolean flag;
>>      int j;
>> 
>> -    static void m(BadPredicateAfterPartialPeel o1, BadPredicateAfterPartialPeel o2, BadPredicateAfterPartialPeel o, int i4) {
>> +    static void m(BadPredicateAfterPartialPeel o1, BadPredicateAfterPartialPeel o2, BadPredicateAfterPartialPeel o, int i4) throws Exception {
>>          int i1 = 1;
>> 
>>          // To delay partial peeling to the loop opts pass right before CCP
>> -        int i2 = 0;
>> -        for (; i2 < 10; i2 += i1);
>> -        i2 = i2 / 10;
>> +        int i2 = 1;
>> +        // for (; i2 < 10; i2 += i1);
>> +        // i2 = i2 / 10;
>> 
>>          // Simplified during CCP:
>>          int i3 = 2;
>> @@ -63,11 +65,12 @@
>> 
>>          not_inlined1();
>> 
>> -        array[0] = -1;
>>          do {
>>              // peeled section starts here
>>              o.flag = false;
>>              o.j = 0;
>> +
>> +            Objects.checkIndex(0, array.length, null);
>> 
>>              if (b) {
>>                  // The following store will be pinned between
>> @@ -300,7 +303,7 @@
>>          not_inlined4();
>>      }
>> 
>> -    static public void main(String[] args) {
>> +    static public void main(String[] args) throws Exception {
>>          BadPredicateAfterPartialPeel o1 = new BadPredicateAfterPartialPeel();
>>          BadPredicateAfterPartialPeel o2 = new BadPredicateAfterPartialPeel();
>>          for (int i = 0; i < 20000; i++) {
>> 
>>