RFR: 8261812: C2 compilation fails with assert(!had_error) failed: bad dominance [v2]

[Tobias Hartmann]

On Tue, 2 Mar 2021 08:59:17 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> In the testByte() test case, byteField is assigned int values that
>> don't fit in a byte.
>> 
>> obj.byteField = (byte)(1 << i);
>> 
>> At parse time, c2 expands this to (in pseudo code):
>> 
>> int v = (1 << i);
>> obj.byteField = (v << 24) >> 24;
>> 
>> next:
>> 
>> StoreBNode::Ideal() runs and StoreNode::Ideal_sign_extended_input()
>> causes this to become:
>> 
>> obj.byteField = (1 << i);
>> 
>> i is then constant folded to 9:
>> 
>> obj.byteField = 512;
>> 
>> obj doesn't escape and is scalarized. The value of the byteField needs
>> to be recorded in the debug info. Because there are 2 such a store, a
>> Phi is created:
>> 
>> (Phi 512 1024)
>> 
>> The Phi is created with type TypeInt::BYTE. Because that doesn't
>> overlap with the input values, the Phi is transformed to top. The test
>> case then triggers a deoptimization and a crash occurs in the
>> deoptimization code.
>> 
>> The failure of the bug report is not a crash during deoptimization but
>> a c2 crash. The root cause is the same but in that case a chain of Phi
>> is involved and when one becomes top, it causes an uncommon trap to
>> have an input that doesn't dominate the uncommon trap.
>> 
>> I noticed LoadBNode::Ideal() re-inserts the 2 shifts when it returns a
>> value from a Store. I propose the same fix here in the allocation
>> elimination code.
>> 
>> Same problem applies to short and char. They are covered by the fix. I
>> also included boolean even though I don't see how to trigger the
>> failure with it.
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Vladimir's review

Looks good to me.

src/hotspot/share/opto/compile.cpp line 4811:

> 4809:   } else {
> 4810:     assert(bt == T_SHORT, "unexpected narrow type");
> 4811:     result = phase->transform( new LShiftINode(value, phase->intcon(16)) );

There's some excess whitespacing.

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

Marked as reviewed by thartmann (Reviewer).

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