RFR: 8347365: C2: Fix the handling of depends_only_on_test [v2]
Quan Anh Mai
qamai at openjdk.org
Fri Jan 23 17:41:33 UTC 2026
On Fri, 23 Jan 2026 16:22:25 GMT, Roland Westrelin <roland at openjdk.org> wrote:
>> As I understand, this change removes logic that's overly conservative but doesn't address any correctness issue (i.e. there's no crash or incorrect execution that this fixes). Given the new logic is less conservative, there should be cases where the code optimizes better with this change. Would it make sense to add IR test cases to catch regressions?
>
>> @rwestrel Thanks for taking a look, this PR makes it strictly more conservative by pinning more nodes when an `IfNode` is elided. However, this conservativeness is necessary, any optimization that can arise from a node not pinned is incorrect, such as [JDK-8331717](https://bugs.openjdk.org/browse/JDK-8331717) or [JDK-8257822](https://bugs.openjdk.org/browse/JDK-8257822).
>
> But those bugs are fixed. Are there known bugs that this PR fixes?
> The way I understand it, you're replacing some existing logic that addresses this kind of issues with some new logic that works some other way but addresses the same issues.
> Also, with attached test case, current c2 code compiles the method with 2 `DivI` nodes while, with your patch, it can common the 2 identical `DivI` nodes and we end up with one. That feels less conservative. So I'm wondering in what way it is more conservative.
>
> (FYI, I agree with the general direction of this patch)
>
> [TestRedundantDivs.java](https://github.com/user-attachments/files/24824874/TestRedundantDivs.java)
@rwestrel Those bugs are fixed, but the fixes did not address the core issue, and I believe there is a chance a similar issue would surface in the future. As a result, this PR tries to fix those bugs again in a more systematic manner. In essence, the current fixes make it so that a `DivNode` acts similarly to a pinned node, albeit still returns `true` on `depends_only_on_test`. This PR makes it so that `DivNode`s act properly as a `depends_only_on_test` node. So you are right, for division and modulo, this PR relaxes their constraints, and I will add an IR test to verify this relaxation.
For all other nodes, this PR makes them more conservative. This is necessary, as they act the same as `DivNode`s before those fixes, and the reason we only see the failures with `DivNode`s is that they throw a signal when their dependency is violated.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/29158#issuecomment-3791446210
More information about the hotspot-compiler-dev
mailing list