RFR (S) 8150534: C1 compilation fails with "Constant field loads are folded during parsing"

[Vladimir Ivanov]

The failures are caused by ScavengeRootsInCode < 2. In such case, 
compilers don't constant fold object-producing loads, because they can 
lead to embedded oops. But it's fine to constant fold an array length, 
since it produces an int.

I suggest the following adjustments [1]:

   (1) add the following assert:
      if (field->is_constant() && field->is_static()) {
+      assert(PatchALot || ScavengeRootsInCode < 2, "Constant field 
loads are folded during parsing");

   (2) replace the guarantee with an assert (hidden in as_array()); 
verifier should guarantee the arrayref points to an array:

        if (!c->is_null_object()) {
-        guarantee(c->is_array(), "Arraylength should reference the array");
-        set_constant(((ciArray*) c)->length());
+        set_constant(c->as_array()->length());
        }

   (3) add 2 new test modes to the test:

+ * @run main/othervm ... -XX:ScavengeRootsInCode=0
+ * @run main/othervm ... -XX:ScavengeRootsInCode=1

Best regards,
Vladimir Ivanov

[1] diff --git a/src/share/vm/c1/c1_Canonicalizer.cpp 
b/src/share/vm/c1/c1_Canonicalizer.cpp
--- a/src/share/vm/c1/c1_Canonicalizer.cpp
+++ b/src/share/vm/c1/c1_Canonicalizer.cpp
@@ -248,10 +248,10 @@
    } else if ((lf = x->array()->as_LoadField()) != NULL) {
      ciField* field = lf->field();
      if (field->is_constant() && field->is_static()) {
+      assert(PatchALot || ScavengeRootsInCode < 2, "Constant field 
loads are folded during parsing");
        ciObject* c = field->constant_value().as_object();
        if (!c->is_null_object()) {
-        guarantee(c->is_array(), "Arraylength should reference the array");
-        set_constant(((ciArray*) c)->length());
+        set_constant(c->as_array()->length());
        }
      }
    }
diff --git a/test/compiler/c1/CanonicalizeArrayLength.java 
b/test/compiler/c1/CanonicalizeArrayLength.java
--- a/test/compiler/c1/CanonicalizeArrayLength.java
+++ b/test/compiler/c1/CanonicalizeArrayLength.java
@@ -27,6 +27,8 @@
   * @summary C1 crashes in Canonicalizer::do_ArrayLength() after fix 
for JDK-8150102
   * @run main/othervm -XX:CompileThreshold=100 -XX:+TieredCompilation 
-XX:TieredStopAtLevel=1 -XX:-BackgroundCompilation 
CanonicalizeArrayLength
   * @run main/othervm -XX:CompileThreshold=100 -XX:+TieredCompilation 
-XX:TieredStopAtLevel=1 -XX:-BackgroundCompilation -XX:+PatchALot 
CanonicalizeArrayLength
+ * @run main/othervm -XX:CompileThreshold=100 -XX:+TieredCompilation 
-XX:TieredStopAtLevel=1 -XX:-BackgroundCompilation 
-XX:ScavengeRootsInCode=0 CanonicalizeArrayLength
+ * @run main/othervm -XX:CompileThreshold=100 -XX:+TieredCompilation 
-XX:TieredStopAtLevel=1 -XX:-BackgroundCompilation 
-XX:ScavengeRootsInCode=1 CanonicalizeArrayLength
   *
   */

On 2/24/16 8:25 PM, Aleksey Shipilev wrote:
> Hi,
>
> In JDK-8150102, we assumed that all suitable constants are exposed as
> Constants after parsing, and added the assert to test that invariant is
> true. And of course, it asserts in some corner cases which we cannot
> reliably reproduce.
>
> With no clear way to fix the underlying issue, we would like to
> re-introduce the deleted branch in Canonicalizer:
>    https://bugs.openjdk.java.net/browse/JDK-8150534
>    http://cr.openjdk.java.net/~shade/8150534/webrev.01/
>
> Note that it fixes the case when -XX:+PatchALot is used, which
> definitely asserts. Some tests unreliably assert even with
> out-of-the-box config. I forked the investigation about the Constant
> field loads to:
>    https://bugs.openjdk.java.net/browse/JDK-8150547
>
> Testing: regression tests; JPRT (in progress)
>
> Thanks,
> -Aleksey
>