Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
d0b9b4a
random bailouts
danielogh Mar 18, 2024
bf2d06b
fixup and whitespace
danielogh Mar 18, 2024
9adda89
add explicit skip in parse.hpp; prevent twice-recorded failure in cha…
danielogh Mar 26, 2024
c44c536
tidy up
danielogh Mar 28, 2024
42b8836
post merge cleanup
danielogh May 8, 2024
4ded3d6
remove flag constraint
danielogh May 14, 2024
ad97f6c
add a basic test
danielogh May 14, 2024
5ebc2b2
whitespace
danielogh May 14, 2024
e71c357
fix test condition
danielogh May 14, 2024
9bd2b17
import
danielogh May 14, 2024
b2b8108
remove warning check entirely
danielogh May 14, 2024
f1fd6b9
make test quicker
danielogh May 14, 2024
d90e806
lower stress seed init again
danielogh May 15, 2024
092df75
revert last
danielogh May 15, 2024
da4e5f7
remove unused var
danielogh May 16, 2024
9245b2c
undo raised initialization; skip if stress seed is not initialized yet.
danielogh Jun 10, 2024
fbcf582
WIP enable in debug too
danielogh Jun 18, 2024
fe407f1
add new flag to CtwRunner
danielogh Jun 18, 2024
d514485
rename secondary flag
danielogh Jun 18, 2024
56cc64f
indentation
danielogh Jun 18, 2024
1a17c69
change conditional
danielogh Jun 18, 2024
2708831
add bool for stress seed initialization state
danielogh Jun 18, 2024
4c7cf93
rename bools and make one DEBUG_ONLY
danielogh Jun 18, 2024
fac4810
init
danielogh Jun 19, 2024
da8d20e
remove new assert
danielogh Jun 19, 2024
7b15e17
try debug-only
danielogh Jun 19, 2024
31f075a
remove once-used method arg
danielogh Jun 20, 2024
43eb389
tweak probability in CtwRunner, add CaptureBailoutInformation
danielogh Jun 20, 2024
4bbd8ab
remaining review comments
danielogh Jul 1, 2024
79da03c
whitespace
danielogh Jul 1, 2024
29642d2
propagate external flags to test
danielogh Jul 1, 2024
26bffb7
rename secondary flag v2
danielogh Jul 1, 2024
f1751b0
tweak requires
danielogh Jul 1, 2024
c2a4304
+1 whitespace
danielogh Jul 2, 2024
c5b1223
merge
danielogh Sep 17, 2024
0da2e6d
Tweak CtwRunner.java; debug only bool flag
danielogh Sep 17, 2024
db32550
words
danielogh Sep 17, 2024
d3f300f
+1 case
danielogh Sep 18, 2024
88d8081
nextInt
danielogh Sep 19, 2024
9ab95aa
phrasing
danielogh Sep 19, 2024
c23e4a4
merge
danielogh Sep 24, 2024
14f6475
remove stress seed initialization bool
danielogh Sep 24, 2024
d91bc06
left over
danielogh Sep 24, 2024
cb748fb
use failing_internal instead; add a const; clarify skip
danielogh Oct 3, 2024
b6eb9a8
Apply suggestions from code review
danielogh Oct 7, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/hotspot/share/opto/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,10 @@ PhaseCFG::PhaseCFG(Arena* arena, RootNode* root, Matcher& matcher)
Node *x = new GotoNode(nullptr);
x->init_req(0, x);
_goto = matcher.match_tree(x);
assert(_goto != nullptr, "");
assert(_goto != nullptr || C->failure_is_artificial(), "");
if (C->failing()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new flag found several missing failing() checks, which are added in this patch.

return;
}
_goto->set_req(0,_goto);

// Build the CFG in Reverse Post Order
Expand Down
8 changes: 8 additions & 0 deletions src/hotspot/share/opto/c2_globals.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@
develop(bool, StressMethodHandleLinkerInlining, false, \
"Stress inlining through method handle linkers") \
\
develop(bool, StressBailout, false, \
"Perform bailouts randomly at C2 failing() checks") \
\
develop(uint, StressBailoutMean, 100000, \
"The expected number of failing() checks made until " \
"a random bailout.") \
range(1, max_juint) \
\
Copy link
Member

Choose a reason for hiding this comment

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

"Interval" is usually a time period. Maybe StressBailoutProbability ?

develop(intx, OptoPrologueNops, 0, \
"Insert this many extra nop instructions " \
"in the prologue of every nmethod") \
Expand Down
6 changes: 6 additions & 0 deletions src/hotspot/share/opto/chaitin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,9 @@ void PhaseChaitin::Register_Allocate() {
}

uint new_max_lrg_id = Split(_lrg_map.max_lrg_id(), &split_arena); // Split spilling LRG everywhere
if (C->failing()) {
return;
}
_lrg_map.set_max_lrg_id(new_max_lrg_id);
// Bail out if unique gets too large (ie - unique > MaxNodeLimit - 2*NodeLimitFudgeFactor)
// or we failed to split
Expand Down Expand Up @@ -551,6 +554,9 @@ void PhaseChaitin::Register_Allocate() {
return;
}
uint new_max_lrg_id = Split(_lrg_map.max_lrg_id(), &split_arena); // Split spilling LRG everywhere
if (C->failing()) {
return;
}
_lrg_map.set_max_lrg_id(new_max_lrg_id);
// Bail out if unique gets too large (ie - unique > MaxNodeLimit - 2*NodeLimitFudgeFactor)
C->check_node_count(2 * NodeLimitFudgeFactor, "out of nodes after split");
Expand Down
44 changes: 34 additions & 10 deletions src/hotspot/share/opto/compile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ Compile::Compile( ciEnv* ci_env, ciMethod* target, int osr_bci,
}

if (StressLCM || StressGCM || StressIGVN || StressCCP ||
StressIncrementalInlining || StressMacroExpansion || StressUnstableIfTraps) {
StressIncrementalInlining || StressMacroExpansion || StressUnstableIfTraps || StressBailout) {
initialize_stress_seed(directive);
}

Expand Down Expand Up @@ -798,7 +798,7 @@ Compile::Compile( ciEnv* ci_env, ciMethod* target, int osr_bci,
assert(failure_reason() != nullptr, "expect reason for parse failure");
stringStream ss;
ss.print("method parse failed: %s", failure_reason());
record_method_not_compilable(ss.as_string());
record_method_not_compilable(ss.as_string() DEBUG_ONLY(COMMA true));
return;
}
GraphKit kit(jvms);
Expand Down Expand Up @@ -973,7 +973,7 @@ Compile::Compile( ciEnv* ci_env,
_types = new (comp_arena()) Type_Array(comp_arena());
_node_hash = new (comp_arena()) NodeHash(comp_arena(), 255);

if (StressLCM || StressGCM) {
if (StressLCM || StressGCM || StressBailout) {
initialize_stress_seed(directive);
}

Expand Down Expand Up @@ -1018,6 +1018,7 @@ void Compile::Init(bool aliasing) {

#ifdef ASSERT
_phase_optimize_finished = false;
_phase_verify_ideal_loop = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to disable the stress mode during PhaseIdealLoop::verify(). I am not sure if we need to enable the stress mode during this verification pass, and it did not seem trivial to make it work. What do you think?

_exception_backedge = false;
_type_verify = nullptr;
#endif
Expand Down Expand Up @@ -1108,7 +1109,7 @@ void Compile::Init(bool aliasing) {
#ifdef ASSERT
// Verify that the current StartNode is valid.
void Compile::verify_start(StartNode* s) const {
assert(failing() || s == start(), "should be StartNode");
assert(failing_internal() || s == start(), "should be StartNode");
}
#endif

Expand All @@ -1118,7 +1119,7 @@ void Compile::verify_start(StartNode* s) const {
* the ideal graph.
*/
StartNode* Compile::start() const {
assert (!failing(), "Must not have pending failure. Reason is: %s", failure_reason());
assert (!failing_internal() || C->failure_is_artificial(), "Must not have pending failure. Reason is: %s", failure_reason());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having the stress mode in debug builds requires weakening asserts since debug builds assert these paths are not taken.

for (DUIterator_Fast imax, i = root()->fast_outs(imax); i < imax; i++) {
Node* start = root()->fast_out(i);
if (start->is_Start()) {
Expand Down Expand Up @@ -2114,7 +2115,7 @@ void Compile::inline_incrementally(PhaseIterGVN& igvn) {
igvn_worklist()->ensure_empty(); // should be done with igvn

while (inline_incrementally_one()) {
assert(!failing(), "inconsistent");
assert(!failing_internal() || failure_is_artificial(), "inconsistent");
}
if (failing()) return;

Expand Down Expand Up @@ -2157,7 +2158,7 @@ void Compile::process_late_inline_calls_no_inline(PhaseIterGVN& igvn) {
igvn_worklist()->ensure_empty(); // should be done with igvn

while (inline_incrementally_one()) {
assert(!failing(), "inconsistent");
assert(!failing_internal() || failure_is_artificial(), "inconsistent");
}
if (failing()) return;

Expand Down Expand Up @@ -2944,6 +2945,9 @@ void Compile::Code_Gen() {

// Build a proper-looking CFG
PhaseCFG cfg(node_arena(), root(), matcher);
if (failing()) {
return;
}
_cfg = &cfg;
{
TracePhase tp("scheduler", &timers[_t_scheduler]);
Expand Down Expand Up @@ -4329,7 +4333,7 @@ void Compile::verify_graph_edges(bool no_dead_code) {
// to backtrack and retry without subsuming loads. Other than this backtracking
// behavior, the Compile's failure reason is quietly copied up to the ciEnv
// by the logic in C2Compiler.
void Compile::record_failure(const char* reason) {
void Compile::record_failure(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures)) {
if (log() != nullptr) {
log()->elem("failure reason='%s' phase='compile'", reason);
}
Expand All @@ -4339,6 +4343,8 @@ void Compile::record_failure(const char* reason) {
if (CaptureBailoutInformation) {
_first_failure_details = new CompilationFailureInfo(reason);
}
} else {
assert(!StressBailout || allow_multiple_failures, "should have handled previous failure.");
}

if (!C->failure_reason_is(C2Compiler::retry_no_subsuming_loads())) {
Expand Down Expand Up @@ -4366,7 +4372,9 @@ Compile::TracePhase::TracePhase(const char* name, elapsedTimer* accumulator)
}

Compile::TracePhase::~TracePhase() {
if (_compile->failing()) return;
if (_compile->failing_internal()) {
return; // timing code, not stressing bailouts.
}
#ifdef ASSERT
if (PrintIdealNodeCount) {
tty->print_cr("phase name='%s' nodes='%d' live='%d' live_graph_walk='%d'",
Expand Down Expand Up @@ -5057,6 +5065,22 @@ bool Compile::randomized_select(int count) {
return (random() & RANDOMIZED_DOMAIN_MASK) < (RANDOMIZED_DOMAIN / count);
}

#ifdef ASSERT
// Failures are geometrically distributed with probability 1/StressBailoutMean.
bool Compile::fail_randomly() {
if ((random() % StressBailoutMean) != 0) {
return false;
}
record_failure("StressBailout");
return true;
}

bool Compile::failure_is_artificial() {
assert(failing_internal(), "should be failing");
return C->failure_reason_is("StressBailout");
}
#endif

CloneMap& Compile::clone_map() { return _clone_map; }
void Compile::set_clone_map(Dict* d) { _clone_map._dict = d; }

Expand Down Expand Up @@ -5144,7 +5168,7 @@ void Compile::sort_macro_nodes() {
}

void Compile::print_method(CompilerPhaseType cpt, int level, Node* n) {
if (failing()) { return; }
if (failing_internal()) { return; } // failing_internal to not stress bailouts from printing code.
EventCompilerPhase event(UNTIMED);
if (event.should_commit()) {
CompilerEvent::PhaseEvent::post(event, C->_latest_stage_start_counter, cpt, C->_compile_id, level);
Expand Down
37 changes: 33 additions & 4 deletions src/hotspot/share/opto/compile.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,8 @@ class Compile : public Phase {
DEBUG_ONLY(Unique_Node_List* _modified_nodes;) // List of nodes which inputs were modified
DEBUG_ONLY(bool _phase_optimize_finished;) // Used for live node verification while creating new nodes

DEBUG_ONLY(bool _phase_verify_ideal_loop;) // Are we in PhaseIdealLoop verification?

// Arenas for new-space and old-space nodes.
// Swapped between using _node_arena.
// The lifetime of the old-space nodes is during xform.
Expand Down Expand Up @@ -786,6 +788,12 @@ class Compile : public Phase {
void set_post_loop_opts_phase() { _post_loop_opts_phase = true; }
void reset_post_loop_opts_phase() { _post_loop_opts_phase = false; }

#ifdef ASSERT
bool phase_verify_ideal_loop() const { return _phase_verify_ideal_loop; }
void set_phase_verify_ideal_loop() { _phase_verify_ideal_loop = true; }
void reset_phase_verify_ideal_loop() { _phase_verify_ideal_loop = false; }
#endif

bool allow_macro_nodes() { return _allow_macro_nodes; }
void reset_allow_macro_nodes() { _allow_macro_nodes = false; }

Expand Down Expand Up @@ -815,7 +823,7 @@ class Compile : public Phase {
ciEnv* env() const { return _env; }
CompileLog* log() const { return _log; }

bool failing() const {
bool failing_internal() const {
return _env->failing() ||
_failure_reason.get() != nullptr;
}
Expand All @@ -827,18 +835,39 @@ class Compile : public Phase {

const CompilationFailureInfo* first_failure_details() const { return _first_failure_details; }

Copy link
Member

Choose a reason for hiding this comment

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

  • Why guarantees? Why not assert? Do we really want this stress code in release builds?
  • If assert, the skip parameter can be DEBUG_ONLY.
  • In any case, I would rename it since is not very clear without examining the source code now. We don't skip the error check, we override the random stress bailout check. E.g. something like DEBUG_ONLY(bool no_stress_bailout) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling the stress code in debug builds too could work. I can give it a try, but it will require some time for more testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling the new flag in debug builds works if we weaken some existing asserts to allow artificial failures. I'm considering making this stress mode debug-only.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally support making the stress mode work in debug builds, this is the mode in which we would get most value out of the bailout stress functionality. Support for product builds is secondary in my opinion (although it may add some value, e.g. helping reproduce issues that only surface in product builds), and may be excluded if there are any concerns about e.g. stability.

bool failing() {
if (failing_internal()) {
return true;
}
#ifdef ASSERT
// Disable stress code for PhaseIdealLoop verification (would have cascading effects).
if (phase_verify_ideal_loop()) {
return false;
}
if (StressBailout) {
return fail_randomly();
}
#endif
return false;
}

#ifdef ASSERT
bool fail_randomly();
bool failure_is_artificial();
#endif

Copy link
Member

@tstuefe tstuefe Jun 11, 2024

Choose a reason for hiding this comment

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

Does this function really have to be inline? We only exercise it for StressBailout=true

bool failure_reason_is(const char* r) const {
return (r == _failure_reason.get()) ||
(r != nullptr &&
_failure_reason.get() != nullptr &&
strcmp(r, _failure_reason.get()) == 0);
}

void record_failure(const char* reason);
void record_method_not_compilable(const char* reason) {
void record_failure(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures = false));
void record_method_not_compilable(const char* reason DEBUG_ONLY(COMMA bool allow_multiple_failures = false)) {
env()->record_method_not_compilable(reason);
// Record failure reason.
record_failure(reason);
record_failure(reason DEBUG_ONLY(COMMA allow_multiple_failures));
}
bool check_node_count(uint margin, const char* reason) {
if (oom()) {
Expand Down
8 changes: 4 additions & 4 deletions src/hotspot/share/opto/gcm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1512,8 +1512,8 @@ void PhaseCFG::schedule_late(VectorSet &visited, Node_Stack &stack) {
C->record_failure(C2Compiler::retry_no_subsuming_loads());
} else {
// Bailout without retry when (early->_dom_depth > LCA->_dom_depth)
assert(false, "graph should be schedulable");
C->record_method_not_compilable("late schedule failed: incorrect graph");
assert(C->failure_is_artificial(), "graph should be schedulable");
C->record_method_not_compilable("late schedule failed: incorrect graph" DEBUG_ONLY(COMMA true));
}
return;
}
Expand Down Expand Up @@ -1693,8 +1693,8 @@ void PhaseCFG::global_code_motion() {
Block* block = get_block(i);
if (!schedule_local(block, ready_cnt, visited, recalc_pressure_nodes)) {
if (!C->failure_reason_is(C2Compiler::retry_no_subsuming_loads())) {
assert(false, "local schedule failed");
C->record_method_not_compilable("local schedule failed");
assert(C->failure_is_artificial(), "local schedule failed");
C->record_method_not_compilable("local schedule failed" DEBUG_ONLY(COMMA true));
}
_regalloc = nullptr;
return;
Expand Down
10 changes: 7 additions & 3 deletions src/hotspot/share/opto/graphKit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,9 @@ static inline void add_one_req(Node* dstphi, Node* src) {
// having a control input of its exception map, rather than null. Such
// regions do not appear except in this function, and in use_exception_state.
void GraphKit::combine_exception_states(SafePointNode* ex_map, SafePointNode* phi_map) {
if (failing()) return; // dying anyway...
if (failing_internal()) {
return; // dying anyway...
}
JVMState* ex_jvms = ex_map->_jvms;
assert(ex_jvms->same_calls_as(phi_map->_jvms), "consistent call chains");
assert(ex_jvms->stkoff() == phi_map->_jvms->stkoff(), "matching locals");
Expand Down Expand Up @@ -446,7 +448,7 @@ void GraphKit::combine_exception_states(SafePointNode* ex_map, SafePointNode* ph

//--------------------------use_exception_state--------------------------------
Node* GraphKit::use_exception_state(SafePointNode* phi_map) {
if (failing()) { stop(); return top(); }
if (failing_internal()) { stop(); return top(); }
Node* region = phi_map->control();
Node* hidden_merge_mark = root();
assert(phi_map->jvms()->map() == phi_map, "sanity: 1-1 relation");
Expand Down Expand Up @@ -2056,7 +2058,9 @@ Node* GraphKit::uncommon_trap(int trap_request,
ciKlass* klass, const char* comment,
bool must_throw,
bool keep_exact_action) {
if (failing()) stop();
if (failing_internal()) {
stop();
}
if (stopped()) return nullptr; // trap reachable?

// Note: If ProfileTraps is true, and if a deopt. actually
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/opto/graphKit.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class GraphKit : public Phase {

#ifdef ASSERT
~GraphKit() {
assert(failing() || !has_exceptions(),
assert(failing_internal() || !has_exceptions(),
"unless compilation failed, user must call transfer_exceptions_into_jvms");
}
#endif
Expand Down Expand Up @@ -182,6 +182,7 @@ class GraphKit : public Phase {

// Tell if the compilation is failing.
bool failing() const { return C->failing(); }
bool failing_internal() const { return C->failing_internal(); }

// Set _map to null, signalling a stop to further bytecode execution.
// Preserve the map intact for future use, and return it back to the caller.
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/opto/lcm.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,7 @@ bool PhaseCFG::schedule_local(Block* block, GrowableArray<int>& ready_cnt, Vecto
// to the Compile object, and the C2Compiler will see it and retry.
C->record_failure(C2Compiler::retry_no_subsuming_loads());
} else {
assert(false, "graph should be schedulable");
assert(C->failure_is_artificial(), "graph should be schedulable");
}
// assert( phi_cnt == end_idx(), "did not schedule all" );
return false;
Expand Down
4 changes: 3 additions & 1 deletion src/hotspot/share/opto/loopnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4935,7 +4935,9 @@ void PhaseIdealLoop::verify() const {
bool success = true;

PhaseIdealLoop phase_verify(_igvn, this);
if (C->failing()) return;
if (C->failing_internal()) {
return;
}

// Verify ctrl and idom of every node.
success &= verify_idom_and_nodes(C->root(), &phase_verify);
Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/opto/loopnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,9 @@ class PhaseIdealLoop : public PhaseTransform {
_verify_only(verify_me == nullptr),
_mode(LoopOptsVerify),
_nodes_required(UINT_MAX) {
DEBUG_ONLY(C->set_phase_verify_ideal_loop();)
build_and_optimize();
DEBUG_ONLY(C->reset_phase_verify_ideal_loop();)
}
#endif

Expand Down
Loading