On Thu, 13 Nov 2025 20:41:56 GMT, Dhamoder Nalla <[email protected]> wrote:
> This PR Introduces an optimized AArch64 intrinsic for Math.log using > reciprocal refinement and a table-driven polynomial. > Improves throughput for double logarithms while preserving IEEE-754 corner > case behavior (±0, subnormals, negatives, NaN). Drive-by, cannot promise a full review. But I'm interested ;) Mostly, I have questions about testing. Are there already tests for accuracy somewhere? Do you have any benchmark results to support this PR? It would be good if we had a way to prove that performance is good for all sorts of inputs. I suppose we don't have any loops here, so we should just make sure to benchmark cases so that all possible paths of the intrinsic are covered, right? src/hotspot/cpu/aarch64/c1_LIRGenerator_aarch64.cpp line 835: > 833: break; > 834: case vmIntrinsics::_dlog: > 835: // Math.log intrinsic is not implemented on AArch64 (see > JDK-8210858), Drive-by comment, and since you are removing this comment: What is the state of https://bugs.openjdk.org/browse/JDK-8210858 ? src/hotspot/cpu/aarch64/macroAssembler_aarch64_log.cpp line 3: > 1: /* Copyright (c) 2018, Cavium. All rights reserved. (By BELLSOFT) > 2: * Copyright (c) 2016, 2021, Intel Corporation. All rights reserved. > 3: * Intel Math Library (LIBM) Source Code Are the dates supposed to be updated? Maybe not, just asking. test/jdk/java/lang/Math/TestLogMinValue.java line 28: > 26: * @bug 8308776 > 27: * @build Tests > 28: * @summary Compare Math.log and StrictMath.log for Double.MIN_VALUE > (denormal smallest positive) to ensure consistency. Are there tests that check for consistency of the other values? test/jdk/java/lang/Math/TestLogMonotonicity.java line 53: > 51: double nv = v * 2.0; > 52: if (nv == v) > 53: break; Suggestion: if (nv == v) { break; } test/jdk/java/lang/Math/TestLogMonotonicity.java line 61: > 59: // Powers of two 2^1 .. 2^16 > 60: for (int i = 1; i <= 16; i++) { > 61: list.add(Math.pow(2.0, i)); It seems you now only cover powers of 2, right? Is this sufficient? I don't know what other tests already exist, so maybe this is already covered elsewhere? ------------- PR Review: https://git.openjdk.org/jdk/pull/28306#pullrequestreview-3479700917 PR Review Comment: https://git.openjdk.org/jdk/pull/28306#discussion_r2539647684 PR Review Comment: https://git.openjdk.org/jdk/pull/28306#discussion_r2539650297 PR Review Comment: https://git.openjdk.org/jdk/pull/28306#discussion_r2539667697 PR Review Comment: https://git.openjdk.org/jdk/pull/28306#discussion_r2539657565 PR Review Comment: https://git.openjdk.org/jdk/pull/28306#discussion_r2539669991
