RFR: 8321204: C2: assert(false) failed: node should be in igvn hash table

[Christian Hagedorn]

On Fri, 5 Apr 2024 07:45:20 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

> Over recent years, we spuriously hit the "node should be in igvn hash table" assert but could never reproduce it. I suspect that this is an extremely rare case where `Node::hash` is 0 which is equivalent to `Node::NO_HASH` and we therefore return false from `NodeHash::hash_delete`. The hash depends on "random" factors like `Node` pointers and it being zero is unfortunate but harmless:
> https://github.com/openjdk/jdk/blob/f26e4308992d989d71e7fbfaa3feb95f0ea17c06/src/hotspot/share/opto/node.hpp#L1114-L1118
> 
> I verified this by hardcoding the `ConstraintCastNode::hash()` to 0 which immediately triggers the assert.
> 
> Although the value of the assert which was added by [JDK-8024070](https://bugs.openjdk.org/browse/JDK-8024070) in JDK 9 is questionable, I think it's worth keeping it but revert the additional printing added by [JDK-8312218](https://bugs.openjdk.org/browse/JDK-8312218).
> 
> I also executed some extensive testing with `Node::hash` hardcoded to zero and found some additional issues that I will address with separate bugs. We might want to add a stress flag to trigger this and similar hash collisions more often.
> 
> Thanks,
> Tobias

Good catch! I've already thought we'll never get to the root of this problem. The fix looks good.
 
> Although the value of the assert which was added by [JDK-8024070](https://bugs.openjdk.org/browse/JDK-8024070) in JDK 9 is questionable, I think it's worth keeping it but revert the additional printing added by [JDK-8312218](https://bugs.openjdk.org/browse/JDK-8312218).

That's reasonable and an easy tweak to the assert.

> I also executed some extensive testing with Node::hash hardcoded to zero and found some additional issues that I will address with separate bugs. We might want to add a stress flag to trigger this and similar hash collisions more often.

Nice, good idea! Stress testing the zero hash would definitely be beneficial and makes this very rare case much more common.

-------------

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18647#pullrequestreview-1982352785