8060036: C2: CmpU nodes can end up with wrong type information

[Andreas Eriksson]

On 2015-05-21 16:11, Roland Westrelin wrote:
>> I did it that way because all the other
>>
>> if (...)
>>   worklist.push(p)
>>
>> in that function omit the curly braces.
>> Should I have them in this change anyway?
> Some c2 code doesn’t (yet) follow the current style. The usual practice is to:
>
> 1) use the current style for new code
> 2) fix the style around what you’ve modified
>
>>> I can sponsor it if you need.
>> That would be very helpful, thanks.
> Can you send me a changeset (hg export) with the properly formatted mercurial comment?
> http://openjdk.java.net/guide/producingChangeset.html
> section "Formatting a Changeset Comment”
> Can you also make sure you’ve run jcheck.

Attached an export, passes jcheck.
I changed the surrounding code to add curly braces and remove extra 
whitespace.
(I don't need an extra round of reviews for that, right?)

- Andreas

>
> Thanks,
> Roland.
>
>> - Andreas
>>
>>> Roland.
>>>
>>>> - Andreas
>>>>
>>>> On 2015-05-19 20:24, Berg, Michael C wrote:
>>>>> Hi Andreas, could you add a comment to the push statement:
>>>>>
>>>>> // Propagate change to user
>>>>>
>>>>> And to Node* p declaration and init:
>>>>>
>>>>> // Propagate changes to uses
>>>>>
>>>>> So that the new code carries all similar information.
>>>>> Else the changes seem ok.
>>>>>
>>>>> -Michael
>>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Andreas Eriksson
>>>>> Sent: Tuesday, May 19, 2015 3:56 AM
>>>>> To: hotspot-compiler-dev at openjdk.java.net
>>>>> Subject: RFR: 8060036: C2: CmpU nodes can end up with wrong type information
>>>>>
>>>>> Hi,
>>>>>
>>>>> Could I please get two reviews for the following bug fix.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8060036
>>>>> http://cr.openjdk.java.net/~aeriksso/8060036/webrev.00/
>>>>>
>>>>> The problem is with type propagation in C2.
>>>>>
>>>>> A CmpU can use type information from nodes two steps up in the graph, when input 1 is an AddI or SubI node.
>>>>> If the AddI/SubI overflows the CmpU node type computation will set up two type ranges based on the inputs to the AddI/SubI instead.
>>>>> These type ranges will then be compared to input 2 of the CmpU node to see if we can make any meaningful observations on the real type of the CmpU.
>>>>> This means that the type of the AddI/SubI can be bottom, but the CmpU can still use the type of the AddI/SubI inputs to get a more specific type itself.
>>>>>
>>>>> The problem with this is that the types of the AddI/SubI inputs can be updated at a later pass, but since the AddI/SubI is of type bottom the propagation pass will stop there.
>>>>> At that point the CmpU node is never made aware of the new types of the AddI/SubI inputs, and it is left with a type that is based on old type information.
>>>>>
>>>>> When this happens other optimizations using the type information can go very wrong; in this particular case a branch to a range_check trap that normally never happened was optimized to always be taken.
>>>>>
>>>>> This fix adds a check in the type propagation logic for CmpU nodes, which makes sure that the CmpU node will be added to the worklist so the type can be updated.
>>>>> I've not been able to create a regression test that is reliable enough to check in.
>>>>>
>>>>> Thanks,
>>>>> Andreas

-------------- next part --------------
# HG changeset patch
# User aeriksso
# Date 1432219751 -7200
#      Thu May 21 16:49:11 2015 +0200
# Node ID 10cc51f8aaa98fa79da3941cf4fb3a6fd3056d75
# Parent  67729f5f33c4a798af42d0e2bcbce838834695f9
8060036: C2: CmpU nodes can end up with wrong type information
Reviewed-by: mcberg, kvn, roland

diff -r 67729f5f33c4 -r 10cc51f8aaa9 src/share/vm/opto/phaseX.cpp
--- a/src/share/vm/opto/phaseX.cpp
+++ b/src/share/vm/opto/phaseX.cpp
@@ -1576,8 +1576,9 @@
         if( m->is_Region() ) {  // New path to Region?  Must recheck Phis too
           for (DUIterator_Fast i2max, i2 = m->fast_outs(i2max); i2 < i2max; i2++) {
             Node* p = m->fast_out(i2); // Propagate changes to uses
-            if( p->bottom_type() != type(p) ) // If not already bottomed out
+            if(p->bottom_type() != type(p)) { // If not already bottomed out
               worklist.push(p); // Propagate change to user
+            }
           }
         }
         // If we changed the receiver type to a call, we need to revisit
@@ -1587,12 +1588,30 @@
         if (m->is_Call()) {
           for (DUIterator_Fast i2max, i2 = m->fast_outs(i2max); i2 < i2max; i2++) {
             Node* p = m->fast_out(i2);  // Propagate changes to uses
-            if (p->is_Proj() && p->as_Proj()->_con == TypeFunc::Control && p->outcnt() == 1)
+            if (p->is_Proj() && p->as_Proj()->_con == TypeFunc::Control && p->outcnt() == 1) {
               worklist.push(p->unique_out());
+            }
           }
         }
         if( m->bottom_type() != type(m) ) // If not already bottomed out
           worklist.push(m);     // Propagate change to user
+
+        // CmpU nodes can get their type information from two nodes up in the
+        // graph (instead of from the nodes immediately above). Make sure they
+        // are added to the worklist if nodes they depend on are updated, since
+        // they could be missed and get wrong types otherwise.
+        uint m_op = m->Opcode();
+        if (m_op == Op_AddI || m_op == Op_SubI) {
+          for (DUIterator_Fast i2max, i2 = m->fast_outs(i2max); i2 < i2max; i2++) {
+            Node* p = m->fast_out(i2); // Propagate changes to uses
+            if (p->Opcode() == Op_CmpU) {
+              // Got a CmpU which might need the new type information from node n.
+              if(p->bottom_type() != type(p)) { // If not already bottomed out
+                worklist.push(p); // Propagate change to user
+              }
+            }
+          }
+        }
       }
     }
   }