RFR: JDK-8284944: assert(cnt++ < 40) failed: infinite cycle in loop optimization [v2]

[Tobias Holenstein]

On Tue, 24 May 2022 14:11:13 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>>> As I commented in bug report, it took 54 (cnt == 52) iterations to finish compilation TestMaxLoopOptsCountReached::test method. Should we just rise default LoopOptsCount flag's value and limit in the assert?
>> 
>> I think the problem is to determine what a good value should be. I'm afraid we could probably come up with some other cases where we can reach the limit again and hit the assert with a larger value. I agree that there might be a problem with `major_progess` and we should not do so many optimizations. But in general, I'm not sure how we can prove that we will only hit the assert in case of a real bug and not just a false positive. Maybe we would need some additional heuristics to make a decision (like handling the live node limit)? This would need some more careful investigation.
>> 
>> The way the assert is currently implemented does not help us. It will never be hit without explicitly disabling partial peeling. So, I think we should either remove it or change its value to 39 to possibly let it fail on the very last loop opts iteration. However, since we already have some different cases where we would fail with 39, this is currently not a good option. But I agree with Vladimir that it would be good to have some mechanism to warn us about such problems in the future.
>> 
>> So, I think there are 2 problems to solve: 
>> - Investigating the known cases so far and figure out why they need so many loop opts iterations.
>> - Make the assert useful again (blocked by the first problem).
>> 
>> I'd suggest to investigate the cases we've found so far and file separate bugs for them if they turn out to be real bugs. Additionally, I suggest the following 2 options:
>> - Remove the assert for now (given that it has never worked as it was supposed to work) and file an RFE to check if we can reintroduce it once the known cases/bugs are fixed and we have some confidence to not hit it again (could include tweaking the `LoopOptsCount` max value or introducing some other heuristics to avoid false positives).
>> - Keep the assert as we have it today and just defer this bug and treat it like the RFE above.
>> 
>> I would opt for option 1 given that the assert has no real value as of today.
>> 
>> What do you think?
>> 
>> Thanks,
>> Christian
>
>> * Remove the assert for now (given that it has never worked as it was supposed to work) and file an RFE to check if we can reintroduce it once the known cases/bugs are fixed and we have some confidence to not hit it again (could include tweaking the `LoopOptsCount` max value or introducing some other heuristics to avoid false positives).
>> * Keep the assert as we have it today and just defer this bug and treat it like the RFE above.
>> 
>> I would opt for option 1 given that the assert has no real value as of today.
> 
> Okay, I agree.

Thanks @vnkozlov and @chhagedorn for you feedback. I will file a follow-up RFE to reintroduce a better check for loop optimization progress and a Bug for the infinite loop in loop optimizations.

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

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