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

[Nils Eliasson]

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