RFR 8227494 [lworld][c1] Optimize withfield bytecode into putfield

[Ioi Lam]

Hi Tobias, thanks for the review.


On 7/25/2019 1:59 AM, Tobias Hartmann wrote:
> Hi Ioi,
>
> this looks good to me!
>
> Why is the change in c1_GraphBuilder.cpp:1879 necessary?

I have modified the line to the following:

   if (!holder->is_loaded()
       || needs_patching /* FIXME: 8228634 - field_modify->will_link() 
may incorrectly return false */
       ) {

I'll add more descriptions into the bug, but so far it seems to affect 
only with __Withfield() macros that operates on the final fields of 
other classes. It doesn't affect "normal" Java code that initializes the 
fields of an inline class in its own <init> method.

Thanks
- Ioi

> Best regards,
> Tobias
>
> On 24.07.19 07:30, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8227494
>> http://cr.openjdk.java.net/~iklam/valhalla/8227494-withfield-optimization.v01/
>>
>>
>> This optimization significantly reduces the amount of code generated by C1 for
>> withfield bytecodes.
>>
>> In the <init> methods of value classes, it's quite common to have a sequence like
>> this, where we load a value object from a local slot, and overwrite the same local
>> slot with a modified copy of the value object.
>>
>>       defaultvalue #1 // class compiler/valhalla/valuetypes/MyValue1
>>       astore 9
>>       ...
>>       iload_0
>>       aload 9
>>       swap
>>       withfield #7 // Field x:I
>>       astore 9
>>
>> If this object was created by defaultvalue, and has not escaped, and is not stored
>> in any other local slots, we can effectively treat the withfield/astore
>> sequence as a single putfield bytecode.
>>
>> I implemented a very rudimentary escape analysis in c1_GraphBuilder.cpp. It seems like
>> there's no need to follow back branches like this:
>>
>>          static FooValue test4() {
>>              FooValue v = FooValue.default;
>>              for (int i=1; i<=2; i++) {
>>                  v = __WithField(v.x, i);
>>                  v = __WithField(v.y, i);
>>                  set_foo_static_if_null(v);
>>              }
>>
>>              return v;
>>          }
>>
>> The local slot for "v" in the loop body is a Phi node (not a NewValueTypeInstance node),
>> so it will fail the "obj->is_optimizable_for_withfield()" check (which will only return true
>> from NewValueTypeInstance) in GraphBuilder::withfield().
>>
>> Thanks
>> - Ioi
>>