8353290: C2: Refactor PhaseIdealLoop::is_counted_loop()

[tabjy]

This PR refactors PhaseIdealLoop::is_counted_loop() into (mostly) CountedLoopConverter::is_counted_loop() and CountedLoopConverter::convert() to decouple the detection and conversion code. This enables us to try different loop configurations easily and finally convert once a counted loop is found.

A nested PhaseIdealLoop::CountedLoopConverter class is created to handle the context, but I'm not if this is the best name or place for it. Please let me know what you think.

Blocks JDK-8336759.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8353290: C2: Refactor PhaseIdealLoop::is_counted_loop() (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24458/head:pull/24458
$ git checkout pull/24458

Update a local copy of the PR:
$ git checkout pull/24458
$ git pull https://git.openjdk.org/jdk.git pull/24458/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24458

View PR using the GUI difftool:
$ git pr show -t 24458

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24458.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kxu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@tabjy The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[tabjy]

/reviewers 2

C2 changes

[openjdk]

@tabjy
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[mlbridge]

Webrevs

• 25: Full - Incremental (6b5254f9)
• 24: Full (4ab9a0e5)
• 23: Full - Incremental (d8baf0fc)
• 22: Full - Incremental (392a010d)
• 21: Full (584a6968)
• 20: Full - Incremental (78ec0a7d)
• 19: Full (de71e7c8)
• 18: Full - Incremental (7395eb99)
• 17: Full - Incremental (ead1ab34)
• 16: Full - Incremental (0a3fff1b)
• 15: Full - Incremental (9005b864)
• 14: Full - Incremental (80c2a62a)
• 13: Full - Incremental (1da190a5)
• 12: Full - Incremental (2cf1da18)
• 11: Full (b1d27675)
• 10: Full - Incremental (01899d64)
• 09: Full - Incremental (a17bfb28)
• 08: Full - Incremental (5fd98f48)
• 07: Full (763adeda)
• 06: Full (345c6ccc)
• 05: Full (fd93998b)
• 04: Full - Incremental (10635a07)
• 03: Full - Incremental (25cfe289)
• 02: Full - Incremental (4d7738c8)
• 01: Full - Incremental (b72e7714)
• 00: Full (3db745d2)

[chhagedorn]

That's a good idea to refactor this, thanks for tackling it! Some initial general comments, I will have a closer look at your patch later again.

[chhagedorn]

Was out last week but I'm seeing your last commit mentions WIP. Let me know when it's ready to have another look again :-)

[tabjy]

@chhagedorn Sorry this took longer than expected. I left a few replies under some of your specific comments. All other issues were addressed. Thank you!

[chhagedorn]

No worries, thanks @tabjy for addressing my suggestions and comments! I won't be able to continue this week but will have another look next week.

[chhagedorn]

Nice update, it looks very good already! I did a complete pass and left some more comments.

I'm a bit worried about breaking something which might not be noticed because we just don't create a counted loop anymore. Have you thought about some ways to test this? One idea could be to do some runs with some custom logging in place when a counted loop was successfully created and then compare the output to a baseline without your patch and the same logging in place. We should just have some confidence that we do not introduce (performance) regressions.

[tabjy]

@chhagedorn: Have you thought about some ways to test this? One idea could be to do some runs with some custom logging in place when a counted loop was successfully created and then compare the output to a baseline without your patch and the same logging in place.

That's a very good suggestion. I tried logging the method id where a counted loop is detected and converted and run tier2_ctw with it. However, the the results are not stable between multiple runs (everytime I get a more or less counted loops detected, even with the old code). There was some non-determinism either in the HS or the JCL code. I couldn't identify them.

Alternatively, I cherry-picked the old counted loop implementation back in with minimum changes and asserted the old and new implementation always produce the same result. This is done on another branch [0] to avoid complicating reviews. All of the tier1 tests pass on GHA [0] and tier2_ctw pass locally as well (I pinkly swear!).

https://github.com/tabjy/jdk/blob/8c1a8c02574af9e2a7b073fc729b3474f187d361/src/hotspot/share/opto/loopnode.cpp#L3010-L3016

So yes, I'm quite confident there's no regressions.

[0] tabjy/jdk@counted-loop-refactor...tabjy:jdk:counted-loop-refactor-log

[1] https://github.com/tabjy/jdk/actions/runs/19149109113

[chhagedorn]

Thanks for the (ongoing?) updates! Let me know, when it's ready to be reviewed again 👍

[tabjy]

@chhagedorn It is ready for review. Please see #24458 (comment) and #24458 (comment). Thanks!

[chhagedorn]

Was too busy this week, will try to come back to this next week!

[chhagedorn]

Thanks for the updates! I first did some skimming first but ended up doing another complete pass. It's already in a good state but I still found some more small things which I left as comments. But then I think it's good to go from my side (regarding the code) :-)

About testing:

Thanks for doing an extended testing with directly inserting the old code again to have a proper comparison. I first thought it's going to be too tricky which is why I proposed a logging - but I already feared that it's not going to be stable enough. So, I'm glad that you managed to do a old vs. new state!

For next steps, I suggest I'll give your patch a spin in our standard testing once you addressed the last comments in this badge. Then I'm also happy to run some more extended testing with your old vs. new counted loop transformation state (would be nice if you can update your branch with the latest review and also merge in latest master).

Let me know if you need some help :-)

[tabjy]

@chhagedorn Thank you reviewing. I'm glad to hear I'm making progress. Please see my pervious comment regarding iteratively uncasting xphi().

[...] give your patch a spin in our standard testing [...]

Yes please. I've addressed last few suggestions and merged in the master.

[...] run some more extended testing with your old vs. new counted loop transformation state [...]

Good idea. I've updated the old vs. new code based on the latest patch on this pr. Please find it on the counted-loop-refactor-old-vs-new branch.

Please let me know how the testing goes. Thank you very much once again!

[chhagedorn]

Thanks @tabjy for the update. Was too busy this week with the mainline fork but I'm happy to take another look next week :-)