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

[Roland Westrelin]

On Mon, 6 May 2024 11:56:52 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> @rwestrel I feel like I am heavily stepping on your toes now....
>> Can you please do refactoring in a separate prior PR? This change is now 3K+ lines, and even reading through it all takes me more than a day, I simply cannot commit this many hours at a time.
>> 
>> I'm thinking in particular about your most recent changes with:
>> - `class Invariance`
>> - `estimate_if_peeling_possible`
>> 
>> Don't get me wrong: I like those refactorings, but they should be done separately.
>> 
>> If you can find anything else that could be done separately, that would help greatly.
>> 
>> I have been painstakingly separating my SuperWord PR's into more reviewable patches, and I do get quicker reviews that way.
>> 
>> My concern: I think the code is now in a state that can be understood (if one spends a day reading it all), but it is hard for me to say that it is correct. If I now approve this patch, then a subsequent reviewer will pay less attention, hence, I feel like I cannot just approve it too quickly now.
>> 
>> If I am too annoying, feel free to ask someone else to review and I will just step back. Maybe @theRealAph wants to review for a while?
>
>> I'm thinking in particular about your most recent changes with:
>> 
>>     * `class Invariance`
>> 
>>     * `estimate_if_peeling_possible`
>> 
>> 
>> Don't get me wrong: I like those refactorings, but they should be done separately.
> 
> The problem I see is that they have little value unless this patch is integrated as it is. What if another reviewer thinks it's better to keep everything related to loop predication together? There's no need to change the class `Invariance` then.

> @rwestrel one idea to split things here:
> 
>     * Early inline of ScopedValue methods
> 
>     * Parsing to IR nodes and expansion back.
> 
>     * Optimization
> 
>     * Tests
> 
> 
> This way, I can spend only a few hours on one at a time, and we can get this done. Of course, you cannot really integrate an individual one, maybe there is a way to use skara for dependent PR's?

Would one commit per line above work? Or do you think it needs to be different PRs?

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

PR Comment: https://git.openjdk.org/jdk/pull/16966#issuecomment-2095855714