[lworld] RFR: 8334484: [lworld] new translation strategy for instance field initializers [v3]

[Maurizio Cimadamore]

On Sat, 29 Mar 2025 01:06:05 GMT, Vicente Romero <vromero at openjdk.org> wrote:

>> Proxy locals are added as a new compiler phase which is executed just before code generation.
>> 
>> Q - what it does?
>> A - basically for code like:
>> 
>> abstract value class Super {
>>     Super(String s) {}
>> }
>> 
>> value class V extends Super {
>>     int i;
>>     V(String s) {
>>         if (s != null) {
>>             i = 100;
>>         } else {
>>             i = 200;
>>         }
>>         super("100" + s);
>>     }
>> }
>> 
>> javac will generate for V's constructor something in the lines of:
>> 
>>     V(String s) {
>>         /*synthetic*/ int local$i;
>>         if (s != null) {
>>             local$i = 100;
>>         } else {
>>             local$i = 200;
>>         }
>>         {
>>             /*synthetic*/ final String local$0 = "100" + s;
>>             i = local$i;
>>             super(local$0);
>>         }
>>     }
>> 
>> so given a constructor for which any strict field is assigned to more than once, javac will generate synthetic local variables that will correspond to it and any read or write done to this strict field will be done on the synthetic local variable.
>> 
>> Javac will also generate additional synthetic local variables to hold the arguments, if any, of the super constructor invocation and will assign the strict fields with the current value of the corresponding synthetic local just before invoking the super constructor.
>> 
>> Q - does it interacts with other valhalla features?
>> A - yes it has a direct interaction with the new stackmap table as the new assert_unset_fields entry, in most cases, is not generated now
>> Q- is it on by default?
>> A- yes but there is a hidden option to tell javac not to generate local proxy variables: `noLocalProxyVars` this will allow us to continue using javac for test cases that check for the generation of the assert_unset_fields entry in the stackmap table attribute until we have time to migrate those tests
>> 
>> TIA for any comments
>
> Vicente Romero has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressing review comments

It seems like there's few more cleanup opportunities now that we detect "early" strict field instance reads in `Resolve` (which make things a lot simpler).

Note: a possible different implementation tactic could be to add some info to JCIdent/JCFieldAccess, so that we can record that the accessed field was instance strict, and that access was "early" (or, we could create a special variable symbol to communicate this). This would eliminate the need for a map. And would also be a bit more precise when rewriting -- because we can only rewrite field accesses that have the special tree/symbol -- and leave the other in place.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LocalProxyVarsGen.java line 245:

> 243:         @Override
> 244:         public void visitIdent(JCTree.JCIdent tree) {
> 245:             if (fieldToLocalMap.get(tree.sym) != null) {

How do we tell this not to rewrite a field access if there's no need (e.g. not in early context) ?

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

PR Review: https://git.openjdk.org/valhalla/pull/1403#pullrequestreview-2729285940
PR Review Comment: https://git.openjdk.org/valhalla/pull/1403#discussion_r2021041870