6756768: C1 generates invalid code

Reviewed-by: kvn, jrose

hotspot/src/share/vm/c1/c1_GraphBuilder.cpp

@@ -676,21 +676,6 @@ GraphBuilder::ScopeData::ScopeData(ScopeData* parent)
 }
 
 
-void GraphBuilder::kill_field(ciField* field) {
-  if (UseLocalValueNumbering) {
-    vmap()->kill_field(field);
-  }
-}
-
-
-void GraphBuilder::kill_array(Value value) {
-  if (UseLocalValueNumbering) {
-    vmap()->kill_array(value->type());
-  }
-  _memory->store_value(value);
-}
-
-
 void GraphBuilder::kill_all() {
   if (UseLocalValueNumbering) {
     vmap()->kill_all();
@@ -987,8 +972,8 @@ void GraphBuilder::store_indexed(BasicType type) {
     length = append(new ArrayLength(array, lock_stack()));
   }
   StoreIndexed* result = new StoreIndexed(array, index, length, type, value, lock_stack());
-  kill_array(value); // invalidate all CSEs that are memory accesses of the same type
   append(result);
+  _memory->store_value(value);
 }
 
 
@@ -1478,9 +1463,6 @@ void GraphBuilder::access_field(Bytecodes::Code code) {
     case Bytecodes::_putstatic:
       { Value val = pop(type);
         append(new StoreField(append(obj), offset, field, val, true, lock_stack(), state_copy, is_loaded, is_initialized));
-        if (UseLocalValueNumbering) {
-          vmap()->kill_field(field);   // invalidate all CSEs that are memory accesses
-        }
       }
       break;
     case Bytecodes::_getfield :
@@ -1503,7 +1485,6 @@ void GraphBuilder::access_field(Bytecodes::Code code) {
         if (is_loaded) store = _memory->store(store);
         if (store != NULL) {
           append(store);
-          kill_field(field);   // invalidate all CSEs that are accesses of this field
         }
       }
       break;
@@ -1900,6 +1881,8 @@ Instruction* GraphBuilder::append_with_bci(Instruction* instr, int bci) {
       assert(i2->bci() != -1, "should already be linked");
       return i2;
     }
+    ValueNumberingEffects vne(vmap());
+    i1->visit(&vne);
   }
 
   if (i1->as_Phi() == NULL && i1->as_Local() == NULL) {
@@ -1926,14 +1909,8 @@ Instruction* GraphBuilder::append_with_bci(Instruction* instr, int bci) {
     assert(_last == i1, "adjust code below");
     StateSplit* s = i1->as_StateSplit();
     if (s != NULL && i1->as_BlockEnd() == NULL) {
-      // Continue CSE across certain intrinsics
-      Intrinsic* intrinsic = s->as_Intrinsic();
-      if (UseLocalValueNumbering) {
-        if (intrinsic == NULL || !intrinsic->preserves_state()) {
-          vmap()->kill_all();      // for now, hopefully we need this only for calls eventually
-          }
-      }
       if (EliminateFieldAccess) {
+        Intrinsic* intrinsic = s->as_Intrinsic();
         if (s->as_Invoke() != NULL || (intrinsic && !intrinsic->preserves_state())) {
           _memory->kill();
         }

hotspot/src/share/vm/c1/c1_GraphBuilder.hpp

@@ -283,8 +283,6 @@ class GraphBuilder VALUE_OBJ_CLASS_SPEC {
   Dependencies* dependency_recorder() const; // = compilation()->dependencies()
   bool direct_compare(ciKlass* k);
 
-  void kill_field(ciField* field);
-  void kill_array(Value value);
   void kill_all();
 
   ValueStack* lock_stack();

hotspot/src/share/vm/c1/c1_ValueMap.hpp

@@ -133,53 +133,77 @@ class ValueNumberingVisitor: public InstructionVisitor {
   virtual void kill_array(ValueType* type) = 0;
 
   // visitor functions
-  void do_StoreField     (StoreField*      x) { kill_field(x->field()); };
+  void do_StoreField     (StoreField*      x) {
-  void do_StoreIndexed   (StoreIndexed*    x) { kill_array(x->type()); };
+    if (!x->is_initialized()) {
-  void do_MonitorEnter   (MonitorEnter*    x) { kill_memory(); };
+      kill_memory();
-  void do_MonitorExit    (MonitorExit*     x) { kill_memory(); };
+    } else {
-  void do_Invoke         (Invoke*          x) { kill_memory(); };
+      kill_field(x->field());
-  void do_UnsafePutRaw   (UnsafePutRaw*    x) { kill_memory(); };
+    }
-  void do_UnsafePutObject(UnsafePutObject* x) { kill_memory(); };
+  }
-  void do_Intrinsic      (Intrinsic*       x) { if (!x->preserves_state()) kill_memory(); };
+  void do_StoreIndexed   (StoreIndexed*    x) { kill_array(x->type()); }
+  void do_MonitorEnter   (MonitorEnter*    x) { kill_memory(); }
+  void do_MonitorExit    (MonitorExit*     x) { kill_memory(); }
+  void do_Invoke         (Invoke*          x) { kill_memory(); }
+  void do_UnsafePutRaw   (UnsafePutRaw*    x) { kill_memory(); }
+  void do_UnsafePutObject(UnsafePutObject* x) { kill_memory(); }
+  void do_Intrinsic      (Intrinsic*       x) { if (!x->preserves_state()) kill_memory(); }
 
-  void do_Phi            (Phi*             x) { /* nothing to do */ };
+  void do_Phi            (Phi*             x) { /* nothing to do */ }
-  void do_Local          (Local*           x) { /* nothing to do */ };
+  void do_Local          (Local*           x) { /* nothing to do */ }
-  void do_Constant       (Constant*        x) { /* nothing to do */ };
+  void do_Constant       (Constant*        x) { /* nothing to do */ }
-  void do_LoadField      (LoadField*       x) { /* nothing to do */ };
+  void do_LoadField      (LoadField*       x) {
-  void do_ArrayLength    (ArrayLength*     x) { /* nothing to do */ };
+    if (!x->is_initialized()) {
-  void do_LoadIndexed    (LoadIndexed*     x) { /* nothing to do */ };
+      kill_memory();
-  void do_NegateOp       (NegateOp*        x) { /* nothing to do */ };
+    }
-  void do_ArithmeticOp   (ArithmeticOp*    x) { /* nothing to do */ };
+  }
-  void do_ShiftOp        (ShiftOp*         x) { /* nothing to do */ };
+  void do_ArrayLength    (ArrayLength*     x) { /* nothing to do */ }
-  void do_LogicOp        (LogicOp*         x) { /* nothing to do */ };
+  void do_LoadIndexed    (LoadIndexed*     x) { /* nothing to do */ }
-  void do_CompareOp      (CompareOp*       x) { /* nothing to do */ };
+  void do_NegateOp       (NegateOp*        x) { /* nothing to do */ }
-  void do_IfOp           (IfOp*            x) { /* nothing to do */ };
+  void do_ArithmeticOp   (ArithmeticOp*    x) { /* nothing to do */ }
-  void do_Convert        (Convert*         x) { /* nothing to do */ };
+  void do_ShiftOp        (ShiftOp*         x) { /* nothing to do */ }
-  void do_NullCheck      (NullCheck*       x) { /* nothing to do */ };
+  void do_LogicOp        (LogicOp*         x) { /* nothing to do */ }
-  void do_NewInstance    (NewInstance*     x) { /* nothing to do */ };
+  void do_CompareOp      (CompareOp*       x) { /* nothing to do */ }
-  void do_NewTypeArray   (NewTypeArray*    x) { /* nothing to do */ };
+  void do_IfOp           (IfOp*            x) { /* nothing to do */ }
-  void do_NewObjectArray (NewObjectArray*  x) { /* nothing to do */ };
+  void do_Convert        (Convert*         x) { /* nothing to do */ }
-  void do_NewMultiArray  (NewMultiArray*   x) { /* nothing to do */ };
+  void do_NullCheck      (NullCheck*       x) { /* nothing to do */ }
-  void do_CheckCast      (CheckCast*       x) { /* nothing to do */ };
+  void do_NewInstance    (NewInstance*     x) { /* nothing to do */ }
-  void do_InstanceOf     (InstanceOf*      x) { /* nothing to do */ };
+  void do_NewTypeArray   (NewTypeArray*    x) { /* nothing to do */ }
-  void do_BlockBegin     (BlockBegin*      x) { /* nothing to do */ };
+  void do_NewObjectArray (NewObjectArray*  x) { /* nothing to do */ }
-  void do_Goto           (Goto*            x) { /* nothing to do */ };
+  void do_NewMultiArray  (NewMultiArray*   x) { /* nothing to do */ }
-  void do_If             (If*              x) { /* nothing to do */ };
+  void do_CheckCast      (CheckCast*       x) { /* nothing to do */ }
-  void do_IfInstanceOf   (IfInstanceOf*    x) { /* nothing to do */ };
+  void do_InstanceOf     (InstanceOf*      x) { /* nothing to do */ }
-  void do_TableSwitch    (TableSwitch*     x) { /* nothing to do */ };
+  void do_BlockBegin     (BlockBegin*      x) { /* nothing to do */ }
-  void do_LookupSwitch   (LookupSwitch*    x) { /* nothing to do */ };
+  void do_Goto           (Goto*            x) { /* nothing to do */ }
-  void do_Return         (Return*          x) { /* nothing to do */ };
+  void do_If             (If*              x) { /* nothing to do */ }
-  void do_Throw          (Throw*           x) { /* nothing to do */ };
+  void do_IfInstanceOf   (IfInstanceOf*    x) { /* nothing to do */ }
-  void do_Base           (Base*            x) { /* nothing to do */ };
+  void do_TableSwitch    (TableSwitch*     x) { /* nothing to do */ }
-  void do_OsrEntry       (OsrEntry*        x) { /* nothing to do */ };
+  void do_LookupSwitch   (LookupSwitch*    x) { /* nothing to do */ }
-  void do_ExceptionObject(ExceptionObject* x) { /* nothing to do */ };
+  void do_Return         (Return*          x) { /* nothing to do */ }
-  void do_RoundFP        (RoundFP*         x) { /* nothing to do */ };
+  void do_Throw          (Throw*           x) { /* nothing to do */ }
-  void do_UnsafeGetRaw   (UnsafeGetRaw*    x) { /* nothing to do */ };
+  void do_Base           (Base*            x) { /* nothing to do */ }
-  void do_UnsafeGetObject(UnsafeGetObject* x) { /* nothing to do */ };
+  void do_OsrEntry       (OsrEntry*        x) { /* nothing to do */ }
-  void do_UnsafePrefetchRead (UnsafePrefetchRead*  x) { /* nothing to do */ };
+  void do_ExceptionObject(ExceptionObject* x) { /* nothing to do */ }
-  void do_UnsafePrefetchWrite(UnsafePrefetchWrite* x) { /* nothing to do */ };
+  void do_RoundFP        (RoundFP*         x) { /* nothing to do */ }
-  void do_ProfileCall    (ProfileCall*     x) { /* nothing to do */ };
+  void do_UnsafeGetRaw   (UnsafeGetRaw*    x) { /* nothing to do */ }
-  void do_ProfileCounter (ProfileCounter*  x) { /* nothing to do */ };
+  void do_UnsafeGetObject(UnsafeGetObject* x) { /* nothing to do */ }
+  void do_UnsafePrefetchRead (UnsafePrefetchRead*  x) { /* nothing to do */ }
+  void do_UnsafePrefetchWrite(UnsafePrefetchWrite* x) { /* nothing to do */ }
+  void do_ProfileCall    (ProfileCall*     x) { /* nothing to do */ }
+  void do_ProfileCounter (ProfileCounter*  x) { /* nothing to do */ }
+};
+
+
+class ValueNumberingEffects: public ValueNumberingVisitor {
+ private:
+  ValueMap*     _map;
+
+ public:
+  // implementation for abstract methods of ValueNumberingVisitor
+  void          kill_memory()                    { _map->kill_memory(); }
+  void          kill_field(ciField* field)       { _map->kill_field(field); }
+  void          kill_array(ValueType* type)      { _map->kill_array(type); }
+
+  ValueNumberingEffects(ValueMap* map): _map(map) {}
 };
 
 

hotspot/test/compiler/6756768/Test6756768.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2008 Sun Microsystems, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/**
+ * @test
+ * @bug 6756768
+ * @summary C1 generates invalid code
+ *
+ * @run main/othervm -Xcomp Test6756768
+ */
+
+class Test6756768a
+{
+    static boolean var_1 = true;
+}
+
+final class Test6756768b
+{
+    static boolean var_24 = false;
+    static int var_25 = 0;
+
+    static boolean var_temp1 = Test6756768a.var_1 = false;
+}
+
+public final class Test6756768 extends Test6756768a
+{
+    final static int var = var_1 ^ (Test6756768b.var_24 ? var_1 : var_1) ? Test6756768b.var_25 : 1;
+
+    static public void main(String[] args) {
+        if (var != 0) {
+            throw new InternalError("var = " + var);
+        }
+    }
+
+}

hotspot/test/compiler/6756768/Test6756768_2.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2008 Sun Microsystems, Inc.  All Rights Reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Sun Microsystems, Inc., 4150 Network Circle, Santa Clara,
+ * CA 95054 USA or visit www.sun.com if you need additional information or
+ * have any questions.
+ */
+
+/**
+ * @test
+ * @bug 6756768
+ * @summary C1 generates invalid code
+ *
+ * @run main/othervm -Xcomp Test6756768_2
+ */
+
+class Test6756768_2a {
+    static int var = ++Test6756768_2.var;
+}
+
+public class Test6756768_2 {
+    static int var = 1;
+
+    static Object d2 = null;
+
+    static void test_static_field() {
+        int v = var;
+        int v2 = Test6756768_2a.var;
+        int v3 = var;
+        var = v3;
+    }
+
+    public static void main(String[] args) {
+        var = 1;
+        test_static_field();
+        if (var != 2) {
+            throw new InternalError();
+        }
+    }
+}