RFR: 8331717: C2: Crash with SIGFPE Because Loop Predication Wrongly Hoists Division Requiring Zero Check [v2]

[Quan Anh Mai]

On Wed, 18 Dec 2024 15:26:20 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>>> IGVN/Idealization will now see that the zero-check-if for 230 DivI is dominated by the zero-check-if for 183 DivI and therefore remove it from the graph and attach 230 DivI to the RangeCheck. This seems a benign and useful transformation in general.
>> 
>> I believe the issue is right there. If a check is removed because it is equivalent to a dominating check, then the `DivI` (as well as all nodes that `depends_only_on_test` needs to be rewired to that dominating check, not to the `Region` that immediately dominates the removed one. In this case, the `230 DivI` needs to be rewired to `175 IfTrue` (the zero check of `183 DivI`), not to `293 RangeCheck`.
>> 
>> If you want to rewire the `DivI` to `293 RangeCheck`, then you need to pin the `DivI`, making it not `depends_only_on_test`. A similar logic can be found in here for array accesses:
>> 
>> https://github.com/openjdk/jdk/blob/8efc5585b74714df6cf8e66853cb63d223534455/src/hotspot/share/opto/ifnode.cpp#L1583
>
>> If you want to rewire the DivI to 293 RangeCheck, then you need to pin the DivI, making it not depends_only_on_test. A similar logic can be found in here for array accesses
> 
> It's probably the most safe way to eventually handle this similarly to array accesses as you suggest. Back there when the checks for `no_dependent_zero_check()` were introduced, we did not have the pinning for array accesses, in place.
> 
> However, for this bug, I think it's better to go with this rather simple point fix and file a follow-up RFE to revisit this and think about aligning it with array accesses with a pinning that's checkable with `depends_only_on_test()` as well. What do you think?

@chhagedorn I think that the wrong control input of the `DivINode` is a potential severe issue that may have more adverse impacts beyond this case. This fix also effectively disables loop predication to run on divisions and their outputs, which may introduce regressions. I believe the case here is peculiar because the zero check is not predicated out of the loop due to failed speculation. As a result, if this issue is not very likely to be hit, I strongly prefer the proper fix of correctly wiring the `230 Div` to `175 IfTrue`. Otherwise, if you want to fix this issue ASAP, then I agree with this patch. Thanks a lot.

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

PR Comment: https://git.openjdk.org/jdk/pull/22666#issuecomment-2551725855