[lworld] RFR: 8266086: [lworld][lw3] C1 produces incorrect code when GlobalValueNumbering is used

[Tobias Hartmann]

On Wed, 28 Apr 2021 18:28:38 GMT, Frederic Parain <fparain at openjdk.org> wrote:

> Please review this fix in C1 GlobalValueNumbering.
> The problem is that C1 doesn't track that a flattened field has been updated when it writes the individual values of this flattened field. The proposed fix is to record the enclosing flattened field with the StoreField node of each individual field, and use this information when the GlobalValueNumbering processes those nodes to kill the flattened field in the ValueMap.
> 
> Tested locally (Linux 64) with hotspot_valhalla (including new unit test) and jdk_valhalla test suites.
> 
> Thank you,
> 
> Fred

Good catch! I've added some comments.

src/hotspot/share/c1/c1_ValueMap.hpp line 152:

> 150:       kill_field(x->field(), x->needs_patching());
> 151:       if (x->enclosing_field() != NULL) {
> 152:         kill_field(x->enclosing_field(), true);

Why do you need `all_offsets == true`? The offset of the enclosing field should always be known, right?

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestC1ValueNumbering.java line 2:

> 1: /*
> 2:  * Copyright (c) 2020, 2020, Oracle and/or its affiliates. All rights reserved.

Copyright should be 2021.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestC1ValueNumbering.java line 32:

> 30:  * @test
> 31:  * @summary Test value numbering behaves correctly with flattened fields
> 32:  * @library /testlibrary /test/lib /compiler/whitebox /

Whitebox is not used.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestC1ValueNumbering.java line 33:

> 31:  * @summary Test value numbering behaves correctly with flattened fields
> 32:  * @library /testlibrary /test/lib /compiler/whitebox /
> 33:  * @compile TestC1ValueNumbering.java

Not required.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestC1ValueNumbering.java line 34:

> 32:  * @library /testlibrary /test/lib /compiler/whitebox /
> 33:  * @compile TestC1ValueNumbering.java
> 34:  * @run main/othervm -Xcomp -XX:TieredStopAtLevel=1 -ea -XX:+UseGlobalValueNumbering

`UseGlobalValueNumbering` is true by default.

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestC1ValueNumbering.java line 40:

> 38: 
> 39: public class TestC1ValueNumbering {
> 40:   static primitive class Point {

4x whitespace indentation should be used for Java code.

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

PR: https://git.openjdk.java.net/valhalla/pull/395