gianm commented on code in PR #19768: URL: https://github.com/apache/druid/pull/19768#discussion_r3990193381
########## processing/src/test/java/org/apache/druid/math/expr/vector/simd/SimdVoMathlibParityTest.java: ########## @@ -0,0 +1,417 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.math.expr.vector.simd; + +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.math.expr.Expr; +import org.apache.druid.math.expr.ExpressionType; +import org.apache.druid.math.expr.vector.ExprEvalDoubleVector; +import org.apache.druid.math.expr.vector.ExprEvalLongVector; +import org.apache.druid.math.expr.vector.ExprEvalVector; +import org.apache.druid.math.expr.vector.ExprVectorProcessor; +import org.apache.druid.math.expr.vector.VectorTestAssertions; +import org.apache.druid.math.expr.vector.functional.DoubleUnivariateDoubleFunction; +import org.apache.druid.math.expr.vector.functional.DoubleUnivariateLongFunction; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * Verifies that the SIMD processors for VO_MATHLIB unary ops (SVML/SLEEF-backed transcendentals) are + * <ol> + * <li>bit-stable across invocations once the JIT has promoted the loop to C2, and</li> + * <li>within a small ulp bound of the scalar {@link Math} equivalent on in-domain inputs.</li> + * </ol> + * + * <p>on JDK 25/AArch64 the C2-compiled SVML/SLEEF dispatch was observed to differ from {@code Math.sin} in ~8% of + * lanes, and to change bits across the C1→C2 tier transition. This test does not attempt to prove + * no-divergence-vs-{@code Math} (there is some, and we accept that for {@code useVectorMathApi=true}); it establishes + * that (a) once the loop is fully warmed, further invocations do not shift again, and (b) the divergence is bounded + * so we can catch regressions if a future JDK/hardware combo drifts substantially. + */ +public class SimdVoMathlibParityTest +{ + // > any tiered-compilation threshold in HotSpot; ensures C2 has compiled the vector loop before + // we start capturing "final" output. + private static final int WARMUP_ITERATIONS = 30_000; + private static final int STABILITY_ITERATIONS = 500; + // Small integer multiple of any reasonable DoubleVector.SPECIES_PREFERRED length (2/4/8) so the SIMD + // loop takes multiple iterations per invocation. + private static final int INPUT_LANES = 64; + // 2 ulps is spec-defensible for the SIMD-vs-scalar comparison: Java's Math is spec'd to be within 1 ulp + // of the correctly-rounded result for most transcendentals (sin/cos/tan/log/exp/asin/acos/atan/cbrt/...); + // the SIMD path targets the same tolerance, so the SIMD-vs-scalar delta can be up to 2 ulps just from + // both landing at opposite ends of their independent 1-ulp windows. sinh/cosh/tanh have no strict Math Review Comment: On my JDK at least, the javadoc for `Math.sinh`, `cosh`, and `tanh` specify that they are within 2.5 ulps of the correct result. ########## sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidOperatorTable.java: ########## @@ -391,6 +391,10 @@ public class DruidOperatorTable implements SqlOperatorTable .add(new DirectOperatorConversion(SqlStdOperatorTable.ACOS, "acos")) .add(new DirectOperatorConversion(SqlStdOperatorTable.ATAN, "atan")) .add(new DirectOperatorConversion(SqlStdOperatorTable.ATAN2, "atan2")) + .add(new DirectOperatorConversion(SqlLibraryOperators.SINH, "sinh")) Review Comment: The new functions should be added to the docs. ########## processing/src/main/java/org/apache/druid/math/expr/vector/simd/SimdSupportedUnaryOp.java: ########## @@ -19,18 +19,117 @@ package org.apache.druid.math.expr.vector.simd; +import org.apache.druid.math.expr.ExpressionProcessing; + /** * Identifies which unary math operations have a {@code jdk.incubator.vector} (SIMD) specialization. Used by * {@link org.apache.druid.math.expr.vector.SimpleVectorMathUnivariateProcessorFactory} subclasses to declare that * their operation can be dispatched to a SIMD variant when the user enables * {@link org.apache.druid.math.expr.ExpressionProcessingConfig#USE_VECTOR_API}. * - * Deliberately does not reference any {@code jdk.incubator.vector} types so that callers wiring the enum into + * <p>Deliberately does not reference any {@code jdk.incubator.vector} types so that callers wiring the enum into * factories do not need the incubator module visible. + * + * <p>Ops annotated as <em>VO_MATHLIB code path</em> below are backed by a vectorized math library (Intel SVML on + * x86, SLEEF on Arm) via {@code VectorOperators.<OP>}, not a direct hardware intrinsic. Performance is + * JVM/build/hardware dependent, typically 1.5x-4x faster than the scalar {@link Math} equivalent on JVMs with + * SVML/SLEEF wired up, but can fall back to a scalar-loop implementation on builds without that wiring. The + * remaining ops (NEG, ABS, SQRT) dispatch to direct hardware FP intrinsics and are consistently faster than + * scalar across every JVM/hardware combo. */ public enum SimdSupportedUnaryOp { - NEG, - ABS, - SQRT + NEG(false), + ABS(false), + SQRT(false), + /** + * Natural log. VO_MATHLIB code path. + */ + LOG(true), + /** + * Natural exponentiation. VO_MATHLIB code path. + */ + EXP(true), + /** + * Base-10 logarithm. VO_MATHLIB code path. + */ + LOG10(true), + /** + * {@code log(1+x)}. VO_MATHLIB code path. + */ + LOG1P(true), + /** + * {@code exp(x)-1}. VO_MATHLIB code path. + */ + EXPM1(true), + /** + * Cube root. VO_MATHLIB code path. + */ + CBRT(true), + /** + * Sine. VO_MATHLIB code path. + */ + SIN(true), + /** + * Cosine. VO_MATHLIB code path. + */ + COS(true), + /** + * Tangent. VO_MATHLIB code path. + */ + TAN(true), + /** + * Arc sine. VO_MATHLIB code path. + */ + ASIN(true), + /** + * Arc cosine. VO_MATHLIB code path. + */ + ACOS(true), + /** + * Arc tangent. VO_MATHLIB code path. + */ + ATAN(true), + /** + * Hyperbolic sine. VO_MATHLIB code path. + */ + SINH(true), + /** + * Hyperbolic cosine. VO_MATHLIB code path. + */ + COSH(true), + /** + * Hyperbolic tangent. VO_MATHLIB code path. + */ + TANH(true); + + private final boolean mathLib; + + SimdSupportedUnaryOp(boolean mathLib) + { + this.mathLib = mathLib; + } + + /** + * Whether this op's SIMD path routes through the JDK's VO_MATHLIB (SVML/SLEEF) dispatch rather than a direct + * hardware FP intrinsic. Callers that gate on + * {@link org.apache.druid.math.expr.ExpressionProcessingConfig#USE_VECTOR_MATH_API} should consult this to + * decide whether the extra flag applies. + */ + public boolean isMathLib() + { + return mathLib; + } + + /** + * Whether SIMD dispatch is currently enabled for this op according to the runtime + * {@link ExpressionProcessing#useVectorApi()} / {@link ExpressionProcessing#useVectorMathApi()} flags. + */ + public boolean isSimdEnabled() Review Comment: Looks like there's one more old call site to convert in `SimpleVectorMathUnivariateProcessorFactory`. ########## processing/src/test/java/org/apache/druid/math/expr/VectorExprResultConsistencyTest.java: ########## @@ -828,11 +829,17 @@ public static void assertEvalsMatch( message ); } else { - Assertions.assertEquals( - nonVectorEval.valueOrThrow()[i], - vectorEval.valueOrThrow()[i], - message - ); + final Object expected = nonVectorEval.valueOrThrow()[i]; + final Object actual = vectorEval.valueOrThrow()[i]; + // Double values compared with ulp-tolerance so the VO_MATHLIB SIMD path can differ from the Review Comment: Is it possible to do this loosening only for the mathlib stuff that requires it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
