RFR: JDK-8258603 c1 IR::verify is expensive [v3]

[Christian Hagedorn]

On Thu, 16 Dec 2021 16:42:35 GMT, Ludvig Janiuk <duke at openjdk.java.net> wrote:

>> IR::verify iterates the whole object graph. This proves costly when used in e.g. BlockMerger inside of iterations over BlockLists, leading to quadratic or worse complexities as a function of bytecode length. In several cases, only a few Blocks were changed, and there was no need to go over the whole graph, but until now there was no less blunt tool for verification than IR::verify.
>> 
>> This PR introduces IR::verify_local, intended to be used when only a defined set of blocks have been modified. As a complement, expand_with_neighbors provides a way to also capture the neighbors of the "modified set" ahead of modification, so that afterwards the appropriate asserts can be made on all blocks which might possibly have been changed. All this should let us remove the expensive IR::verify calls, while still performing equivalent (or stricter) assertions.
>> 
>> Some changes have been made in the verifiers along the way. Some amount of refactoring, and even added invariants (see validate_edge_mutiality).
>
> Ludvig Janiuk has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup2

Thanks for doing the updates and pulling the numbers together! 

> And again, it's important to remember that the point of this fix is to protect against "worst cases" with e.g. functions with very long bytecode. The compile time improvement in the general case, if any, is only a bonus.

Yes, exactly. I think as long as are not noticeably worse on average (which we seem not to be according to your evaluation), we're good to go since this only affects debug builds.

Maybe someone else can also comment on the `ASSERT` vs. `NOT_PRODUCT` usage. I think we should either convert everything to one or the other but not mix it since all the verification code belongs together. Thinking about it again, `ASSERT` might be preferable to not affect the optimized build (as `IR::verify()` was guarded by `ASSERT` before).

src/hotspot/share/c1/c1_IR.cpp line 1401:

> 1399: 
> 1400:     for (int i = 0; i < block->end()->number_of_sux(); i++) {
> 1401:       if (blocks.contains(block->end()->sux_at(i))) { continue; }

I think it's better to move the `continue` to a new line.

src/hotspot/share/c1/c1_Optimizer.cpp line 441:

> 439: 
> 440: #ifndef PRODUCT
> 441:     _hir->verify_local(blocks_to_verify_later);

You can also use `NOT_PRODUCT()` here.

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6850