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

[Vladimir Kozlov]

Thank you, John

My first implementation was exactly as you suggested. But then I realized that separate Value() methods will be better 
since they will match corresponding specialized Ideal() methods which handle values which does not fit (for example, 
LoadBNode::Ideal()).

Logic in LoadNode::Identity() just do bailout and relies on Value() and Ideal() to do transformations.

Thanks,
Vladimir

On 1/18/12 10:49 PM, John Rose wrote:
> 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) {
>