Skip to content

Commit

Permalink
8292602: ZGC: C2 late barrier analysis uses invalid dominator informa…
Browse files Browse the repository at this point in the history
…tion

Co-authored-by: Nils Eliasson <neliasso@openjdk.org>
Reviewed-by: kvn, thartmann
  • Loading branch information
robcasloz and Nils Eliasson committed Sep 23, 2022
1 parent f6d78cd commit eec992c
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 0 deletions.
60 changes: 60 additions & 0 deletions src/hotspot/share/opto/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,48 @@ void PhaseCFG::insert_goto_at(uint block_no, uint succ_no) {
block->_freq = freq;
// add new basic block to basic block list
add_block_at(block_no + 1, block);
// Update dominator tree information of the new goto block.
block->_idom = in;
block->_dom_depth = in->_dom_depth + 1;
if (out->_idom != in) {
// The successor block was not immediately dominated by the predecessor
// block, so there is no dominator subtree to update.
return;
}
// Update immediate dominator of the successor block.
out->_idom = block;
// Increment the dominator tree depth of the goto block's descendants. These
// are found by a depth-first search starting from the successor block. Two
// domination properties guarantee that only descendant blocks are visited:
// 1) all dominators of a block b must appear in any path from the root to b;
// 2) if a block b does not dominate another block b', b cannot dominate any
// block reachable from b' either.
// The exploration uses header indices as block identifiers, since
// Block::_pre_order might not be unique in the context of this function.
ResourceMark rm;
VectorSet descendants;
descendants.set(block->head()->_idx); // The goto block is a descendant of itself.
Block_List worklist;
worklist.push(out); // Start exploring from the successor block.
while (worklist.size() > 0) {
Block* b = worklist.pop();
// The immediate dominator of b is a descendant, hence b is also a
// descendant. Even though all predecessors of b might not have been visited
// yet, we know that all dominators of b have been already visited (since
// they must appear in any path from the goto block to b).
descendants.set(b->head()->_idx);
b->_dom_depth++;
for (uint i = 0; i < b->_num_succs; i++) {
Block* s = b->_succs[i];
if (s != get_root_block() &&
!descendants.test(s->head()->_idx) &&
// Do not search below non-descendant successors, since any block
// reachable from them cannot be descendant either.
descendants.test(s->_idom->head()->_idx)) {
worklist.push(s);
}
}
}
}

// Does this block end in a multiway branch that cannot have the default case
Expand Down Expand Up @@ -1272,6 +1314,23 @@ void PhaseCFG::verify_memory_writer_placement(const Block* b, const Node* n) con
assert(found, "block b is not in n's home loop or an ancestor of it");
}

void PhaseCFG::verify_dominator_tree() const {
for (uint i = 0; i < number_of_blocks(); i++) {
Block* block = get_block(i);
assert(block->_dom_depth <= number_of_blocks(), "unexpected dominator tree depth");
if (block == get_root_block()) {
assert(block->_dom_depth == 1, "unexpected root dominator tree depth");
// The root block does not have an immediate dominator, stop checking.
continue;
}
assert(block->_idom != nullptr, "non-root blocks must have immediate dominators");
assert(block->_dom_depth == block->_idom->_dom_depth + 1,
"the dominator tree depth of a node must succeed that of its immediate dominator");
assert(block->num_preds() > 2 || block->_idom == get_block_for_node(block->pred(1)),
"the immediate dominator of a single-predecessor block must be the predecessor");
}
}

void PhaseCFG::verify() const {
// Verify sane CFG
for (uint i = 0; i < number_of_blocks(); i++) {
Expand Down Expand Up @@ -1341,6 +1400,7 @@ void PhaseCFG::verify() const {
assert(block->_num_succs == 2, "Conditional branch must have two targets");
}
}
verify_dominator_tree();
}
#endif // ASSERT

Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/opto/block.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,8 @@ class PhaseCFG : public Phase {
// Check that block b is in the home loop (or an ancestor) of n, if n is a
// memory writer.
void verify_memory_writer_placement(const Block* b, const Node* n) const NOT_DEBUG_RETURN;
// Check local dominator tree invariants.
void verify_dominator_tree() const NOT_DEBUG_RETURN;
void verify() const NOT_DEBUG_RETURN;
};

Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/opto/compile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2975,6 +2975,7 @@ void Compile::Code_Gen() {
}
cfg.fixup_flow();
cfg.remove_unreachable_blocks();
cfg.verify_dominator_tree();
}

// Apply peephole optimizations
Expand Down
6 changes: 6 additions & 0 deletions src/hotspot/share/opto/idealGraphPrinter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,12 @@ void IdealGraphPrinter::visit_node(Node *n, bool edges, VectorSet* temp_set) {
print_prop("block", C->cfg()->get_block(0)->_pre_order);
} else {
print_prop("block", block->_pre_order);
if (node == block->head()) {
if (block->_idom != NULL) {
print_prop("idom", block->_idom->_pre_order);
}
print_prop("dom_depth", block->_dom_depth);
}
// Print estimated execution frequency, normalized within a [0,1] range.
buffer[0] = 0;
stringStream freq(buffer, sizeof(buffer) - 1);
Expand Down

3 comments on commit eec992c

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GoeLin
Copy link
Member

@GoeLin GoeLin commented on eec992c Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/backport jdk17u-dev

@openjdk
Copy link

@openjdk openjdk bot commented on eec992c Dec 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GoeLin the backport was successfully created on the branch GoeLin-backport-eec992c6 in my personal fork of openjdk/jdk17u-dev. To create a pull request with this backport targeting openjdk/jdk17u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit eec992c6 from the openjdk/jdk repository.

The commit being backported was authored by Roberto Castañeda Lozano on 23 Sep 2022 and was reviewed by Vladimir Kozlov and Tobias Hartmann.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u-dev:

$ git fetch https://github.com/openjdk-bots/jdk17u-dev GoeLin-backport-eec992c6:GoeLin-backport-eec992c6
$ git checkout GoeLin-backport-eec992c6
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u-dev GoeLin-backport-eec992c6

Please sign in to comment.