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

[Tobias Hartmann]

On Tue, 17 Sep 2024 17:13:34 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comments
>
> src/hotspot/share/opto/parse2.cpp line 1385:
> 
>> 1383:   bool do_stress_trap = StressUnstableIfTraps && ((C->random() % 2) == 0);
>> 1384:   if (do_stress_trap) {
>> 1385:     Node* counter_addr = makecon(TypeRawPtr::make((address)&_trap_stress_counter));
> 
> Would it be easier if you use new Ideal macro node for this and expand it in macro expansion phase?

I think the problem with a macro node is that it might prevent optimizations that look through the memory graph, especially since the macro node would need to read and update (raw) memory. Also, such a simple load, increment and store does not pollute the graph too much to justify a macro node, I my opinion.

> src/hotspot/share/opto/parse2.cpp line 1497:
> 
>> 1495:     incr_store = store_to_memory(control(), counter_addr, counter, T_INT, Compile::AliasIdxRaw, MemNode::unordered);
>> 1496:   }
>> 1497: 
> 
> From the glance it looks like the code above. Should you put it into separate method to call it in both places?

Make sense, I factored it out.

> src/hotspot/share/opto/parse2.cpp line 1589:
> 
>> 1587:   // Search for an unstable if trap
>> 1588:   CallStaticJavaNode* trap = nullptr;
>> 1589:   for (int i = 0; i <= 1; ++i) {
> 
> Should we check that it is `IfNode` and it has 2 output edges? May be assert?

I added an assert.

> src/hotspot/share/opto/parse2.cpp line 1590:
> 
>> 1588:   CallStaticJavaNode* trap = nullptr;
>> 1589:   for (int i = 0; i <= 1; ++i) {
>> 1590:     Node* out = orig_iff->raw_out(i)->find_out_with(Op_CallStaticJava);
> 
> Why not cast (CallStaticJava*) here?

Done. I simplified the code a bit further.

> src/hotspot/share/opto/parse2.cpp line 1591:
> 
>> 1589:   for (int i = 0; i <= 1; ++i) {
>> 1590:     Node* out = orig_iff->raw_out(i)->find_out_with(Op_CallStaticJava);
>> 1591:     if (out != nullptr && out->isa_CallStaticJava() && out->as_CallStaticJava()->is_uncommon_trap()) {
> 
> You don't need `out->isa_CallStaticJava()` because `find_out_with()` will return `nullptr` in other cases.

Good catch. Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1764843481
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1764823071
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1764823251
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1764823627
PR Review Comment: https://git.openjdk.org/jdk/pull/21037#discussion_r1764823838