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

[John Rose]

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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20120119/6d0fde46/attachment.html