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

[Roland Westrelin]

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 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.

> 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.