RFR: 8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload

[Aleksey Shipilev]

On Thu, 25 Feb 2021 07:02:45 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> The assert fires because the current IfNode's condition was not
>> canonicalized when it's being folded with a dominating
>> IfNode. idealize_test() should have taken care of that because it
>> executed before fold_compares() but it didn't because:
>> 
>>   Node* new_b = phase->transform( new BoolNode(b->in(1), bt.negate()) );
>>   if( !new_b->is_Bool() ) return NULL;
>> 
>> caused it to bail out. new_b is a constant. This happens because of
>> the order in which nodes are processed by IGVN. The If's current Bool
>> would also constant fold but it's in the IGVN worklist and hasn't been
>> processed yet.
>> 
>> The fix I propose is to keep Aleksey's defensive fix but to check that
>> the Bool input is indeed about to be transformed by IGVN and that that
>> would cause the IfNode to be reprocessed.
>> 
>> I tried to write a test case but didn't succeed. The 2 If nodes come
>> from a tableswitch that's transformed into a series of If based on
>> profile data. I couldn't reproduce the profile data with a simple test
>> case.
>
> src/hotspot/share/opto/ifnode.cpp line 984:
> 
>> 982:       return false;
>> 983:     }
>> 984:     assert(this_bool->_test.is_less() && !fail->_con, "incorrect test");
> 
> This should lead with "this test was canonicalized" comment? Missed during the move, I think.

I also find it a bit weird to even have the assert on this path, as we tested all cases in the if-chain before, and the only path to this assert is through `lt` and `le` -- which is `is_less`? Maybe I am missing something, though.

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

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