RFR: 8271202: C1: assert(false) failed: live_in set of first block must be empty [v2]

[Igor Veresov]

On Tue, 7 Dec 2021 23:25:03 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> I have written a checker which detects usage of the illegal phi function. In case of the reproducer provided in the JBS bug ("Reduced.java"), it finds the following and bails out:
>> 
>> invalidating local 8 because of type mismatch (new_value is NULL)
>> Bailing out because StoreIndexed (id 98) uses illegal phi (id 68)
>> 
>> I haven't checked why that node uses the illegal phi. That still seems to be a bug. Maybe there's a better solution to the underlying problem, but I hope my checker is useful to analyze bugs and to make C1 more resilient.
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add test.

I took a deeper look and I added a comment to bug report.

The root cause seems to be because of the irreducible loops (and therefore an unusual block traversal order when inserting phis) the phi invalidation logic in try_merge() doesn't invalidate phis that have invalid locals as inputs. I've attached a drawing:
[8271202.pdf](https://github.com/openjdk/jdk/files/7739876/8271202.pdf). Notice that i54 = phi (i43, 96) is not invalidated even though 96 is illegal. Transitively, i43, it should be illegal too. I would propose that we add a check for that and bailout in move_phi(). 

Suggested fix: 


diff --git a/src/hotspot/share/c1/c1_LIRGenerator.cpp b/src/hotspot/share/c1/c1_LIRGenerator.cpp
index c064558b458..b386b541f89 100644
--- a/src/hotspot/share/c1/c1_LIRGenerator.cpp
+++ b/src/hotspot/share/c1/c1_LIRGenerator.cpp
@@ -963,6 +963,14 @@ void LIRGenerator::move_to_phi(PhiResolver* resolver, Value cur_val, Value sux_v
   Phi* phi = sux_val->as_Phi();
   // cur_val can be null without phi being null in conjunction with inlining
   if (phi != NULL && cur_val != NULL && cur_val != phi && !phi->is_illegal()) {
+    if (phi->is_local()) {
+      for (int i = 0; i < phi->operand_count(); i++) {
+        Value op = phi->operand_at(i);
+        if (op != NULL && op->type()->is_illegal()) {
+          bailout("illegal phi operand");
+        }
+      }
+    }
     Phi* cur_phi = cur_val->as_Phi();
     if (cur_phi != NULL && cur_phi->is_illegal()) {
       // Phi and local would need to get invalidated

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

PR: https://git.openjdk.java.net/jdk/pull/6683