RFR: 8342941: IGV: Add new graph dumps for post loop, empty loop removal, and one iteration removal [v3]
Christian Hagedorn
chagedorn at openjdk.org
Fri Jul 11 07:20:41 UTC 2025
On Thu, 10 Jul 2025 19:07:30 GMT, Daniel Lundén <dlunden at openjdk.org> wrote:
>> Saranya Natarajan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
>>
>> - fix 2 of review
>> - Merge master
>> - Addressing review comments
>> - Initial Fix
>
> src/hotspot/share/opto/phasetype.hpp line 83:
>
>> 81: flags(AFTER_REMOVE_EMPTY_LOOP, "After Remove Empty Loop") \
>> 82: flags(BEFORE_ONE_ITERATION_LOOP, "Before Replacing One Iteration Loop") \
>> 83: flags(AFTER_ONE_ITERATION_LOOP, "After Replacing One Iteration Loop") \
>
> Very much a nit, but I think this should be "One-Iteration Loop". Or, is it in fact one _iteration loop_ (as it reads now)? Looking at the code, I think it is the former. @chhagedorn can maybe clarify?
>
> This is not specific to your changeset, but also appears in existing source code comments. Maybe a good opportunity to clean this up everywhere?
>
> Also, maybe "Replacing" should be "Replace"? Seems to better fit the style used for other phase names.
One-Iteration loop sounds better indeed. I also agree with the other suggestions.
Something else I've noticed is that we could also benefit when we add dumps for `duplicate_loop_backedge()` which creates a new loop node (i.e. could be seen as "major modification"). I just looked into recently and found myself adding dumps there manually for debugging. I guess since this is a dump adding RFE, we could also add that one. What do you think? But then we would need to update the PR title to something like "add various new graph dumps during loop opts".
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25756#discussion_r2199865344
More information about the hotspot-compiler-dev
mailing list