RFR: 8320649: C2: Optimize scoped values [v13]

[Emanuel Peter]

On Thu, 4 Apr 2024 12:56:21 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   test fix
>
> src/hotspot/share/opto/callGenerator.cpp line 1218:
> 
>> 1216:     // slow_call:
>> 1217:     // result = slowGet();
>> 1218:     // goto continue;
> 
> Now you have duplication of these comments, see above `remove_first_probe_if_when_it_never_hits`. Would it make sense to put this somewhere more "central"?

And you further repeat the comments below. I fear that if someone would eventually make changes, they would not update all comments, and then the comments diverge.

> src/hotspot/share/opto/graphKit.cpp line 4276:
> 
>> 4274:   // carrier thread's cache.
>> 4275:   // return _gvn.transform(LoadNode::make(_gvn, nullptr, immutable_memory(), p, p->bottom_type()->is_ptr(),
>> 4276:   //       TypeRawPtr::NOTNULL, T_ADDRESS, MemNode::unordered));
> 
> This is not dead code, but for the purpose of showing that `immutable_memory` does not work?

Ah, you just moved it. ok.

> src/hotspot/share/opto/loopnode.cpp line 5181:
> 
>> 5179: }
>> 5180: 
>> 5181: bool PhaseIdealLoop::optimize_scoped_value_get_nodes() {
> 
> This is a bit of a monster method, with deep nesting. Hard to read. Can you break it up somehow into smaller methods?

You seem to do an all-vs-all optimization here, right?
Could you do that in a nested loop, and then just dispatch for all combinations:
hits-hits
hits-get
get-hits
get-get

Also: is there a reason for the reverse-order?

> test/hotspot/jtreg/compiler/c2/irTests/TestScopedValue.java line 68:
> 
>> 66:                     "testSlowPath1,testSlowPath2,testSlowPath3,testSlowPath4,testSlowPath5,testSlowPath6,testSlowPath7,testSlowPath8,testSlowPath9,testSlowPath10");
>> 67:             for (String test : tests) {
>> 68:                 TestFramework.runWithFlags("-XX:+TieredCompilation", "--enable-preview", "-XX:CompileCommand=dontinline,java.lang.ScopedValue::slowGet", "-DTest=" + test);
> 
> What is the reason for running each test individually?

Hmm. Profile pollution. But if it is so bad, then won't that be an issue "in the real wold"? Is this test not very artificial?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1551632692
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1551646332
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1551692752
PR Review Comment: https://git.openjdk.org/jdk/pull/16966#discussion_r1551704827