RFR: 8324517: C2: crash in compiled code because of dependency on removed range check CastIIs [v2]

[Roland Westrelin]

On Fri, 3 May 2024 10:18:44 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> src/hotspot/share/opto/compile.cpp line 3906:
>> 
>>> 3904:       for (DUIterator_Fast imax, i = m->fast_outs(imax); i < imax; i++) {
>>> 3905:         Node* use = m->fast_out(i);
>>> 3906:         if (use->is_Mem() || use->Opcode() == Op_DivI || use->Opcode() == Op_DivL) {
>> 
>> `Op_ModI` and `Op_ModL` are missing here. And isn't this too strong in cases where we can prove that the operand is non-zero? Could you re-use `PhaseIterGVN::no_dependent_zero_check`? Please also add corresponding tests.
>> 
>> Looking at `PhaseIterGVN::no_dependent_zero_check`, I noticed that `UDiv[I/L]Node` and `UMod[I/L]Node` are not handled but I think they should. I think this was missed when these nodes where added by [JDK-8282221](https://bugs.openjdk.org/browse/JDK-8282221). One can probably extend @chhagedorn's test from [JDK-8259227](https://bugs.openjdk.org/browse/JDK-8259227) to trigger the same issue.
>
>> `Op_ModI` and `Op_ModL` are missing here.
> 
> Good catch! I added test cases for `Op_ModI` and `Op_ModL` , the unsigned variants and the also the DivMod variants. I also fixed the patch so it handles all of them.
> 
>>  And isn't this too strong in cases where we can prove that the operand is non-zero?
> 
> I don't think it's too strong. The operand can be non zero because of a range check `CastII` somewhere along the subgraph that starts at the node's second input. In that case, `PhaseIterGVN::no_dependent_zero_check` would return true but removing the range `CastII` would cause the bugs that are triggered by the test case.
> 
>> Looking at `PhaseIterGVN::no_dependent_zero_check`, I noticed that `UDiv[I/L]Node` and `UMod[I/L]Node` are not handled but I think they should. I think this was missed when these nodes where added by [JDK-8282221](https://bugs.openjdk.org/browse/JDK-8282221). One can probably extend @chhagedorn's test from [JDK-8259227](https://bugs.openjdk.org/browse/JDK-8259227) to trigger the same issue.
> 
> That seems like a different problem that out of the scope of this particular issue.

I realized that I didn't understand your comment when I replied.
What you're saying, I think, is that if we have, say, a `CastII` that's input to a `DivI` node, if the input to that cast is non zero, then we don't need to add the `CastII` control as dependency to the `DivI`. The problem, I think, is that the `CastII` could be input to say an `AddI` node which would then be input to the `DivI`. What we would then need to know is whether if we remove the `CastII`, the `AddI` is still non null or not. That doesn't seem straightforward because this is done once we have no igvn instance to propagate types anymore. So, while I agree this is conservative, it still seems like the most reasonable fix.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18377#discussion_r1590836071