RFR: 8273277: C2: Move conditional negation into rc_predicate [v3]

[Nils Eliasson]

On Tue, 9 Nov 2021 20:20:10 GMT, Nils Eliasson <neliasso at openjdk.org> wrote:

>> Hi,
>> 
>> I need some feedback on this patch. This was reported from Tencent and found in internal testing about the same time. This patch is based on a a patch provided by Tencent.
>> 
>> In some very specific circumstances we need to negate the range checks that we create in PhaseIdealLoop::loop_predication_impl_helper. This is done in three places, but that method also calls insert_initial_skeleton_predicate where this isn't taken into account. 
>> 
>> To simplify things I have moved the negation logic into rc_predicate. This should prevent us from missing this check again.
>> 
>> I do have a concern that negating the condition of the rangecheck in the skeleton predicate, since the skeleton predicate will be used as a clone template, and some rangechecks optimizations seems to assume that range checks always have LT as the condidtion. On the other hand - it is a really uncommon scenario since we haven't failed here before.
>> 
>> Feedback appreciated.
>> 
>> Best regards,
>> Nils
>
> Nils Eliasson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add test

> > I do have a concern that negating the condition of the rangecheck in the skeleton predicate, since the skeleton predicate will be used as a clone template, and some rangechecks optimizations seems to assume that range checks always have LT as the condition. On the other hand - it is a really uncommon scenario since we haven't failed here before.
> 
> Is there any specific code that you worry about?

I have seen checks for LT, but I see none that specifically would be affecting this change.

> 
> I think it should be fine because the purpose of copying and instantiating skeleton range check predicates is to guarantee that control/data paths die consistently when the main loop induction variable falls outside of the allowed range of an array access. But @rwestrel and @chhagedorn looked more into this recently.
> 
> Can we add the test that Tencent found as well?

Yes - I've added it.

> 
> Please update the affects versions in the bug report.

Done.

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

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