[15] RFR(M): 8237859: C2: Crash when loads float above range check

[Christian Hagedorn]

Thank you Nils for reviewing it again!

Best regards,
Christian

On 25.03.20 14:29, Nils Eliasson wrote:
> Looks good!
> 
> Best regards,
> 
> // Nils
> 
> On 2020-03-20 15:56, Christian Hagedorn wrote:
>> Hi Roland
>>
>> On 20.03.20 11:41, Roland Westrelin wrote:
>>>> This fix addresses this issue and rewires the control input of data
>>>> nodes belonging to the main-loop to a range check predicate before the
>>>> pre-loop to the copied main-loop range check predicates (copied in
>>>> PhaseIdealLoop::copy_skeleton_predicates_to_main_loop_helper). When
>>>> unrolling, the range check predicates of the main-loop are updated
>>>> together with the control edges (done in
>>>> PhaseIdealLoop::update_main_loop_skeleton_predicates). The data nodes
>>>> belonging to the post-loop are rewired to the zero-trip guard If node
>>>> right before the CountedLoopNode (there are no predicates copied 
>>>> down to
>>>> the post-loop). For example, in [5] (in contrast to [4]), we rewired 
>>>> the
>>>> control of 247 LoadP to the main-loop and the control of 377 LoadP to
>>>> the post-loop.
>>>
>>> That doesn't feel right to me. The predicates between the main and pre
>>> loop were added to avoid inconsistencies during the compilation
>>> process. And unless the condition constant folds during loop opts, they
>>> are removed and not included in the final compiled code. But here you
>>> are using them to fix a code generation issue. So you might end up with
>>> loads made dependent on a predicate that's removed and then the loads
>>> would become dependent on the zero trip guard above the main loop.
>>>
>>> Why node make the loads in the main loop dependent on the zero trip
>>> guard right before the main loop the way you do it for the post loop?
>>
>> You're right. I somehow wrote that code with the thought that those 
>> cloned skeleton predicates can fail - but they never do. Therefore, 
>> your suggestion to rewire the control inputs for those main loop data 
>> nodes to the zero trip guard before the main loop sounds reasonable.
>>
>> Here are the updated webrevs with this change:
>> Full (after push of 8240227):
>> http://cr.openjdk.java.net/~chagedorn/8237859/webrev.02/
>> Incr from webrev.incremental.00:
>> http://cr.openjdk.java.net/~chagedorn/8237859/webrev.incremental.01/
>>
>> Best regards,
>> Christian