RFR(S): 8252696: Loop unswitching may cause out of bound array load to be executed

[Christian Hagedorn]

On 03.09.20 17:35, Roland Westrelin wrote:
> 
> Hi Christian,
> 
> Thanks for looking at this.
> 
>> Nice analysis! This fix sounds reasonable to me. Have I understood that
>> correctly that in your testcase, the main loop is just unrolled enough
>> such that the loop nodes can be removed (so no over-unrolling)?
> 
> Right.
> 
>>> [..] and then eliminates the
>>> dominating ones that it finds useless. Removing dominated predicates
>>> that have no control dependent data nodes the way it's done in the
>>> current logic is suspicious: data nodes are sometimes logically
>>> dependent on multiple range checks but control dependent on only one.
>>
>> That's a valid point. Is there a better way we could find out which
>> predicates are useless after cloning them to the slow and fast loop or
>> is it either way not a problem to keep the original ones alive?
> 
> AFAIU, that's not a problem with the patch I propose which only clones
> the skeleton predicates. All of them are required to be copied in case
> of unrolling.

I see, thanks for explaining.

>> I agree, I think I added this back there because I was temporarily
>> modifying the old_new mapping in a non-conform way on L302. So, I just
>> wanted to be sure that everything is reset and works as intended. But
>> I'm also fine with just removing this assertion code.
> 
> Should I remove L302 as well? I'm confused by what's going on here.

No this line should be fine. Just to be sure we are talking about the 
same line, I was referring to L302 in your patch (old_new.map(..)). I 
used the old_new mapping here to get a quick reference from the cloned 
node in the slow loop (when processing the slow loop after the fast 
loop) back to the original node in the fast loop on line L280. But I 
added additional asserts there to ensure that everything is reset as 
intended. So, it's fine that you removed the assertion code on L257-267.

Best regards,
Christian

>> Some small comment:
>> - You probably don't need the assertion on L257 in loopPredicate.cpp as
>> you are already checking it in the if-condition on L253.
> 
> Right. I'll remove it.
> 
>> - Same file, you should update the assert message on L268 to something
>> like "... projection of an If node".
> 
> I'll make that change too.
> 
> Roland.