RFR: 8286800: Assert in PhaseIdealLoop::dump_real_LCA is too strong

[Christian Hagedorn]

On Wed, 28 Sep 2022 19:04:07 GMT, Dhamoder Nalla <duke at openjdk.org> wrote:

> https://bugs.openjdk.org/browse/JDK-8286800
> 
> assert(real_LCA != NULL) in dump_real_LCA is not appropriate in bad graph scenario when both wrong_lca & early nodes are start nodes
> 
> jvm!PhaseIdealLoop::dump_real_LCA():
> // Walk the idom chain up from early and wrong_lca and stop when they intersect.
> while (!n1->is_Start() && !n2->is_Start()) {
> ...
> }
> assert(real_LCA != NULL, "must always find an LCA");
> 
> Fix: replace assert with a console message

Hi @dhanalla 

I don't think we should remove this assertion. We should always be able to find an LCA with given `early` and `wrong_lca`. Hitting the assertion indicates that there is a mistake in the way `dump_real_LCA()` finds the LCA. I therefore suggest to fix the algorithm instead of disabling the assert. 

Back there when I wrote that code, I've tried to simultaneously walk the idom chains from `early` and `wrong_lca` to be more efficient. I don't think this optimization was necessary looking at the added complexity and given that we are in debug code and about to fail anyways.

I'm currently working on [JDK-8285835](https://bugs.openjdk.org/browse/JDK-8285835) where I hit the very same assertion failure. In this process, I've fixed `dump_real_LCA()` and made it easier. I'm also printing less information (I think the idom dumps are too verbose at the moment). If you like, I could take this bug over. Otherwise, I can also follow up with an RFE to get the improved printing in separately.

Cheers,
Christian

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

PR: https://git.openjdk.org/jdk/pull/10472