RFR: 8335334: Stress mode to randomly execute unstable if traps [v3]

[Tobias Hartmann]

On Wed, 18 Sep 2024 18:09:52 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Moved declaration
>
> src/hotspot/share/opto/parse2.cpp line 1375:
> 
>> 1373: 
>> 1374: // Used by StressUnstableIfTraps
>> 1375: static volatile int _trap_stress_counter = 0;
> 
> Please, check that all accesses to it use ExternalAddress (external_word_type relocation).

Checked. We use the same pattern at other places already, for example in `GraphKit::increment_counter`.

> src/hotspot/share/opto/parse2.cpp line 1377:
> 
>> 1375: static volatile int _trap_stress_counter = 0;
>> 1376: 
>> 1377: void Parse::load_trap_stress_counter(Node*& counter, Node*& incr_store) {
> 
> `load_trap_` -> `increment_trap_`

Good point. Renamed.

> src/hotspot/share/opto/parse2.cpp line 1594:
> 
>> 1592:     trap = (CallStaticJavaNode*)orig_iff->raw_out(i)->find_out_with(Op_CallStaticJava);
>> 1593:     if (trap != nullptr && trap->is_uncommon_trap() && trap->jvms()->should_reexecute() &&
>> 1594:         Deoptimization::trap_request_reason(trap->uncommon_trap_request()) == Deoptimization::Reason_unstable_if) {
> 
> Can we use `ProjNode::is_uncommon_trap_if_pattern()` here?

Not directly but we could use `IfNode::uncommon_trap_proj`. Updated the code accordingly.

> src/hotspot/share/opto/parse2.cpp line 1627:
> 
>> 1625:   trap_region->set_req(1, trap_proj);
>> 1626:   trap_region->set_req(2, if_true);
>> 1627:   trap->set_req(0, _gvn.transform(trap_region));
> 
> Can you use `IdealKit::if_then()` here to simplify code?

I thought about that. The problem is that `IdealKit` does not work if we `stopped()` already. And if we added a trap, we stopped in one of the branches, one of which might be the one we are currently parsing (note that we are updating the trap only after creating the branches). Also, we still need to create a new region manually to merge the "should trap" path from the new check and the "should trap" path from the original if into the trap. What do you think?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1766248797
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1766249140
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1766769687
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1766776983