RFR: JDK-8258603 c1 IR::verify is expensive

[Christian Hagedorn]

On Wed, 15 Dec 2021 15:39:26 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).

I think it's a good idea to narrow the verification down to the blocks which are actually changed and also add some additional verification steps. Even with the newly added checks, it should still be better than running a complete check each time. Did you observe a big gain in the C1 compilation time for debug builds with your new patch? Maybe you also want to check again with the tests from https://github.com/openjdk/jdk16/pull/44.

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

> 1263: 
> 1264: inline void validate_end_not_null(BlockBegin* block) {
> 1265:   assert(block->end() != NULL, "Expect block end to exist.");

As this method is just a single assertion and used from one place, you could directly inline it into `block_do()` below.

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

> 1275: typedef GrowableArray<BlockList*> BlockListList;
> 1276: 
> 1277: void verify_successor_xentry_flag(const BlockBegin* block) {

Could this method also be directly inlined into `block_do` below? The class name and assertion message should give enough information about the purpose of the check.

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

> 1292: 
> 1293: // Validation goals:
> 1294: // * code() length == blocks length

Just a minor thing, might be better to use `-` for the itemization instead of `*` as it looks like they belong to a block comment..

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

> 1372: 
> 1373: inline void verify_block_begin_field(BlockBegin* block) {
> 1374:   for ( Instruction *cur = block; cur != NULL; cur = cur->next()) {

Same here, could this method also be directly inlined into `block_do` below? Spacing and asterisk position can be improved here.

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

> 1384: };
> 1385: 
> 1386: void validate_edge_mutuality(BlockBegin* block) {

Same here, could this method also be directly inlined into `block_do` below?

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

> 1415: 
> 1416:     for (int i = 0; i < block->end()->number_of_sux(); i++) {
> 1417:       if (blocks.contains(block->end()->sux_at(i))) continue;

To avoid confusion, I think it's better to always use braces for one-line ifs and loops instead.

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

> 1432: 
> 1433: void IR::verify_local(BlockList& blocks) {
> 1434: #ifdef ASSERT

Do we need an `ASSERT` here? The code is already guarded with  `ifndef PRODUCT`. Same for L1447.

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

> 1447: #ifdef ASSERT
> 1448:   XentryFlagValidator xe;
> 1449:   this->iterate_postorder(&xe);

You can remove `this->`.

src/hotspot/share/c1/c1_IR.hpp line 341:

> 339:   static void print(BlockBegin* start, bool cfg_only, bool live_only = false) PRODUCT_RETURN;
> 340:   void print(bool cfg_only, bool live_only = false)                           PRODUCT_RETURN;
> 341:   void expand_with_neighborhood(BlockList& blocks);

Also needs to be guarded with `PRODUCT_RETURN`.

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

> 181:   }
> 182: 
> 183: #ifdef ASSERT

`expand_with_neighborhood()` is guarded with `ifndef PRODUCT`. You should use the same here.

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

> 259: 
> 260: #ifdef ASSERT
> 261:   _hir->verify_local(blocks_to_verify_later);

You can use `NOT_PRODUCT()` for single line statements (assuming you change the `ASSERT` into `ifndef PRODUCT`).

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

> 409: #endif // ASSERT
> 410: 
> 411: #ifdef ASSERT

Can be merged into one `ifdef` block.

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

Changes requested by chagedorn (Reviewer).

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