On Thu, 10 Sep 2026 17:35:48 GMT, Kangcheng Xu <[email protected]> wrote:
>> Hi, >> >> This PR adds intrinsics to `Preconditions.checkFromToIndex()` and >> `Preconditions.checkFromIndexSize()` to produced optimized IR that uses >> `RangeCheck` node instead of implicit comparisons. >> `Preconditions.checkIndex()` is also refactored (without additional >> optimizations) to use the same helper function. >> >> Some common patterns where calling `checkFromToIndex` or >> `checkFromIndexSize` in a loop can have range checks in main loop eliminated >> completely and, therefore, enables empty loop removal. >> >> IR and correctness tests are included and passing. Additional tests on >> `Preconditions.checkIndex()` were also added. >> >> Thanks! >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Kangcheng Xu has updated the pull request incrementally with two additional > commits since the last revision: > > - simplify logic by allowing deopt with size == 0 > - remove left over code src/hotspot/share/opto/library_call.cpp line 1347: > 1345: > 1346: // 1) size > 0 — strict positive check (size = 0 is valid per spec > but deopt is acceptable) > 1347: Node* casted_size = insert_non_negative_check(*this, size, > BoolTest::gt, bt); Shouldn't you check `casted_size == nullptr` here? src/hotspot/share/opto/library_call.cpp line 1350: > 1348: > 1349: // 2) length >= 0 — non-negative check > 1350: Node* casted_length = insert_non_negative_check(*this, length, > BoolTest::ge, bt); Shouldn't you check `casted_length == nullptr` here? src/hotspot/share/opto/library_call.cpp line 1389: > 1387: // 1) length >= 0 (non-negative guard) > 1388: // 2) from u< length (range check, RCE-hoistable) > 1389: // 3) to - from - 1 u< length (range check, RCE-hoistable) This one checks `from < to`. I think you want to make it clearer in the comment. But if: `from = Integer.MAX_VALUE` `to = Integer.MIN_VALUE` then `to - from - 1 = 0`. So even though `from > to`, `to - from - 1 u< length` is true. This said, it looks harmless because `to - 1 u< length` fails (`to - 1 = Integer.MAX_VALUE` which can't be strictly less than any positive signed integer). Can you confirm, this is correct? src/hotspot/share/opto/library_call.cpp line 1411: > 1409: > 1410: // 1) length >= 0 — non-negative guard > 1411: Node* casted_length = insert_non_negative_check(*this, length, > BoolTest::ge, bt); Shouldn't you check `casted_length == nullptr` here? src/hotspot/share/opto/library_call.cpp line 1434: > 1432: Node* to_minus_one = _gvn.transform(SubNode::make(to, > _gvn.integercon(1, bt), bt)); > 1433: Node* casted_to_minus_one = insert_unsigned_range_check(*this, > to_minus_one, casted_length, bt); > 1434: if (casted_to_minus_one == nullptr) { Couldn't you cast `to` to `[1..length-1]` here? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31138#discussion_r4003571199 PR Review Comment: https://git.openjdk.org/jdk/pull/31138#discussion_r4003573275 PR Review Comment: https://git.openjdk.org/jdk/pull/31138#discussion_r4003784797 PR Review Comment: https://git.openjdk.org/jdk/pull/31138#discussion_r4003592732 PR Review Comment: https://git.openjdk.org/jdk/pull/31138#discussion_r4003677739
