JDK-8325443 : C2 misses fold_compare opportunity in loops
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 21,22,23
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2024-02-07
  • Updated: 2024-02-12
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
tbdUnresolved
Related Reports
Relates :  
Description
I attached a JMH benchmark showcasing the problem. The results on my machine:

Benchmark                             Mode  Cnt   Score    Error  Units
FoldCompare.foldableComparesExternal  avgt    3  26,630 ± 13,493  ns/op
FoldCompare.foldableComparesInline    avgt    3  57,526 ± 10,844  ns/op
FoldCompare.unsignedCompare           avgt    3  26,499 ± 11,984  ns/op 

C2 has code to merge handwritten range checks (e.g. value >= 0 && value < 100) into one CmpU (value <u 100). See https://github.com/openjdk/jdk/blob/c3a632dca75d2fad0a60e03e7b4fc64edb1e906e/src/hotspot/share/opto/ifnode.cpp#L687.

However, this doesn't work if the range check is directly written in a loop (see the foldableComparesInline method in the benchmark). It works if the exactly same check is extracted into an extra method.

From my analysis, the problem stems from the !region->has_phi() check here https://github.com/openjdk/jdk/blob/c3a632dca75d2fad0a60e03e7b4fc64edb1e906e/src/hotspot/share/opto/ifnode.cpp#L760C56-L760C74. In the case of the extracted method, the method gets first inlined, and projections both point to a region that then directly points to another region, but is not associated with a Phi. In contrast, the manually inlined check directly points to a region with a Phi, bailing out at this check.

My understanding of the existing code isn't good enough to tell how this can be improved. I assume the Phi check is too conservative, but I also assume that it is there for a valid reason.
Comments
I agree that it would definitely be worth to revisit this code and try to improve it and support additional cases. By doing so, we could also improve the documentation to state which cases work and which not together with an explanation why.
12-02-2024

Thanks for taking a look. I indeed focused on "why does it work in some cases" rather than trying to understand the original change. However, from my understanding of https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/ifnode.cpp#L687 it was intended from the beginning to cover at least some additional cases, but I might be simply misunderstanding the comment. Anyway, I think it would make sense to expand this to cover more cases, but I don't know how much work that would be.
10-02-2024

Hi [~hgreule], the code was originally written to optimize explicit range checks (also see https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2015-February/017127.html): if (index < 0 || index >= array.length) { throw new ArrayIndexOutOfBoundsException(); } into a single CmpU. So, this code only works when we throw/return/trap when both conditions are met such that we do not merge anything at the region with a phi. The case you are describing is different since both paths are merged again and we need a phi to choose the correct value. But I guess this case could also be optimized - it was just not originally intended to cover that.
09-02-2024