Request for reviews (S): 7131302: connode.cpp:205 Error: ShouldNotReachHere()

[John Rose]

On Jan 18, 2012, at 10:18 PM, Vladimir Kozlov wrote:

> http://cr.openjdk.java.net/~kvn/7131302/webrev
> 
> 7131302: connode.cpp:205 Error: ShouldNotReachHere()
> 
> Split_thru_phi optimization used LoadNode::Value() and incorrectly replaced LoadS(StoreC(65535)) with constant (65535) which does not fit into SHORT type. As result merged Phi type become top.
> 
> Add Value() methods to short and byte Load nodes to truncate constants which does not fit.
> 
> Verified with failed test.

Your fix works but I think it makes it harder to read the resulting code, especially regarding the implicit correlation between Value and Identity.

It is also possible to encounter non-constants with the wrong kind of sign, such as a value in the range 0..65535 for a signed short.  The logic which handles this is in LoadNode::Identity, guarded with "memory_size() < BytesPerInt".

I suggest putting parallel logic into LoadNode::Value, rather than splitting LoadNode::Value.  That will make it easier to see the relation between Ideal and Value.  Below is a rough sketch of what I'm thinking of.

It's easy to see that your proposed change fixes problems with wrongly signed constant nodes popping up, but it doesn't correspond to the similar task of guarding against other sorts of wrongly signed nodes.

Thanks,
-- John

diff --git a/src/share/vm/opto/memnode.cpp b/src/share/vm/opto/memnode.cpp
--- a/src/share/vm/opto/memnode.cpp
+++ b/src/share/vm/opto/memnode.cpp
@@ -1718,8 +1718,24 @@
   bool is_instance = (tinst != NULL) && tinst->is_known_instance_field();
   if (ReduceFieldZeroing || is_instance) {
     Node* value = can_see_stored_value(mem,phase);
-    if (value != NULL && value->is_Con())
-      return value->bottom_type();
+    if (value != NULL && value->is_Con()) {
+      if (memory_size() < BytesPerInt &&
+          !phase->type(value)->higher_equal(phase->type(this))) {
+        // If the input to the store does not fit with the load's result type,
+        // it must be truncated. We can't delay until Ideal call since
+        // a singleton Value is needed for split_thru_phi optimization.
+        int con = value->get_int();
+        switch (Opcode()) {
+        case Op_LoadUB:  con = (jubyte) con; break;
+          ...
+        default:  con = min_jint; break;
+        }
+        if (con != min_jint)
+          return TypeInt::make(con);
+      } else {
+        return value->bottom_type();
+      }
+    }
   }
 
   if (is_instance) {