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

[Vladimir Kozlov]

On 1/19/12 1:13 AM, John Rose wrote:
> On Jan 19, 2012, at 12:57 AM, Vladimir Kozlov wrote:
>
>> 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.
>
> That makes sense. Thanks for the explanation.
>
> It still bothers me that LoadNode::Value can (in principle) return a bogus type. The LoadBNode::Value override appears
> to exclude cases where LoadNode::Value would return garbage, but it seems very difficult to verify this is the case.
> Basically, you've prevented LoadNode::Value from breaking LoadBNode::Value in one known case, but you still call it
> "trustfully" at the end of the block.

I see what you mean, I will add assert in LoadNode::Value():

      if (value != NULL && value->is_Con()) {
+      assert(value->bottom_type()->higher_equal(_type),"sanity");
        return value->bottom_type();
      }

I assume this should be true for all types and not just integer. Right?

Thanks,
Vladimir

>
> You might consider putting in a guard or assert against badly signed types emerging from LoadNode::Value. It could go
> either in LoadNode::Value (parallel to the similar guard in LoadNode::Identity) or at the calls from
> Load[UBS]*Node::Value to LoadNode::Value.
>
> Even if you don't do this, I'm fine with your change, and you can count me as a reviewer.
>
> -- John