fix: fix GEMM correctness defaults and expand CPU SIMD ops - #709
Conversation
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds CPU float/double tiled parallel MATMUL with env-driven controls and tracing; GPU GEMM result validation with Trace diagnostics and CPU fallback; OpenCL exposes device metadata and a double-buffered GEMM fallback; span-based finiteness checks across numeric ops; new CPU/GPU matmul diagnostics and minor allocation reductions. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DirectGpuEngine
participant GPU_Backend
participant Validator
participant CpuEngine
Caller->>DirectGpuEngine: MatrixMultiply(A, B)
DirectGpuEngine->>GPU_Backend: Execute GEMM (CLBlast/dynamic/double-buffered)
GPU_Backend-->>DirectGpuEngine: ResultArray
alt AIDOTNET_GEMM_VALIDATE enabled
DirectGpuEngine->>Validator: IsAnyNonFinite(ResultArray)?
Validator-->>DirectGpuEngine: ok / badIndex
alt invalid
DirectGpuEngine->>CpuEngine: MatrixMultiply(A, B) (CPU fallback)
CpuEngine-->>DirectGpuEngine: CPU_Result
DirectGpuEngine-->>Caller: CPU_Result
else valid
DirectGpuEngine-->>Caller: ResultArray
end
else
DirectGpuEngine-->>Caller: ResultArray
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
changed the title
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (1)
4436-4464: Diagnostic help text: ensure listed env vars are actually implemented (esp.AIDOTNET_GEMM_VALIDATE).
You added help entries forAIDOTNET_GEMM_ENABLE_DYNAMIC,AIDOTNET_GEMM_SAFE,AIDOTNET_GEMM_UNSAFE,AIDOTNET_GEMM_VALIDATE; please confirm each one is consumed in code (in this file or elsewhere) so users don't chase no-op toggles.
🤖 Fix all issues with AI agents
In @src/AiDotNet.Tensors/Engines/CpuEngine.cs:
- Around line 1560-1567: The multiplication in ShouldParallelizeMatMul can
overflow the 64-bit long; replace the naive long product with a non-overflowing
approach by computing the op count as a double (e.g., double ops = (double)m * n
* k) or by performing saturating arithmetic, then compare that double against
CpuMatMulParallelThresholdOps (cast the threshold to double) to decide
parallelization; update the ShouldParallelizeMatMul method (use the function
name to locate it) to use the double-based check (or clamp to long.MaxValue if
you prefer saturating) so large dims don't silently flip the sign and
incorrectly disable parallelization.
🧹 Nitpick comments (7)
src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs (1)
415-429: Consider SIMD vectorization forAllFiniteto match PR's SIMD expansion theme.Since this PR expands CPU SIMD usage, this helper could use
Vector<float>for faster validation on large GEMM results. However, given this is opt-in diagnostics code, the current scalar approach is acceptable.♻️ Optional SIMD-accelerated implementation
private static bool AllFinite(float[] data, out int badIndex) { + int i = 0; + if (System.Numerics.Vector.IsHardwareAccelerated && data.Length >= System.Numerics.Vector<float>.Count) + { + int vectorEnd = data.Length - (data.Length % System.Numerics.Vector<float>.Count); + for (; i < vectorEnd; i += System.Numerics.Vector<float>.Count) + { + var vec = new System.Numerics.Vector<float>(data, i); + // vec - vec yields NaN for Inf values and NaN for NaN values + var diff = vec - vec; + if (!System.Numerics.Vector.EqualsAll(diff, System.Numerics.Vector<float>.Zero)) + { + // Fall back to scalar to find exact index + for (int j = i; j < i + System.Numerics.Vector<float>.Count; j++) + { + if (float.IsNaN(data[j]) || float.IsInfinity(data[j])) + { + badIndex = j; + return false; + } + } + } + } + } + - for (int i = 0; i < data.Length; i++) + for (; i < data.Length; i++) { float value = data[i]; if (float.IsNaN(value) || float.IsInfinity(value)) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/Engines/CpuEngine.cs (2)
37-43: Env-var toggles: consider more forgiving parsing + override for perf knobs.Right now only
"1"enables tracing/single-thread. If you want this to be friendlier in CI, consider accepting"true"/"True"as well, and (optionally) lettingCpuMatMulParallelThresholdOps/tile sizes be overridden via env var for tuning without recompiling.
1569-1606: Minor cleanup: unused params + redundant inner-loop args.
MultiplyFloatBlock/MultiplyDoubleBlocktakembut don’t use it, and the0, jEnd-j0args passed toSimdVector.MatMulInnerLoop*look redundant since you already sliceb/cto[j0..jEnd). If the inner-loop API allows it, consider simplifying to reduce call-site confusion.Also applies to: 1608-1645
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
1624-1685: Packed GEMM:useColumnMajorChardcoded tofalse—either wire it up or remove dead branches.
Right nowuseColumnMajorCis always false, but the method still contains substantial branching to handle column-major C (padding/copy-back). If column-major C is intentionally unsupported in this path, consider deleting the unused branch (or add a comment explaining why it must be false).
1747-1774: Dynamic enable flag parsing: consider usingGetEnvBoolfor consistency (and less surprising UX).
AIDOTNET_GEMM_ENABLE_DYNAMICis checked via== "1"while other env flags in this file supporttrue/false/yes/no/on/off. Switching toGetEnvBoolwould make toggles consistent and easier to use.tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs (1)
93-128: Consider disposing result matrices if they implementIDisposable.The
Comparemethod createscpuResultandgpuResultmatrices. IfMatrix<float>implementsIDisposable, these should be disposed after use to avoid resource leaks, especially when running multiple correctness checks in a loop.♻️ Suggested fix if matrices are disposable
private static (double maxError, double avgError, int nonFiniteCount) Compare( CpuEngine cpuEngine, IEngine gpuEngine, Matrix<float> a, Matrix<float> b) { - var cpuResult = cpuEngine.MatrixMultiply(a, b); - var gpuResult = gpuEngine.MatrixMultiply(a, b); + using var cpuResult = cpuEngine.MatrixMultiply(a, b); + using var gpuResult = gpuEngine.MatrixMultiply(a, b); var cpuSpan = cpuResult.AsSpan(); var gpuSpan = gpuResult.AsSpan();tests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cs (1)
81-90: Consider extracting shared helper to reduce duplication.The
CreateRandomMatrixmethod is nearly identical to the one inGpuMatMulDiagnostics. Consider extracting this to a shared utility class if these diagnostic files evolve further.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/Program.cstests/AiDotNet.Tests/DirectGpuTests.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cstests/AiDotNet.Tests/DirectGpuTests.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/Program.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cstests/AiDotNet.Tests/DirectGpuTests.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/Program.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (24)
src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs (3)
38-39: Configuration approach for GEMM validation is sound.Using an environment variable for opt-in validation with CPU fallback is appropriate for diagnostics/debugging without impacting production performance. The static initialization ensures no repeated environment lookups.
405-409: LGTM!The validation logic correctly short-circuits when disabled, provides useful diagnostic info (first bad index), and gracefully falls back to CPU by returning null.
444-448: LGTM!Consistent validation pattern applied to the cached weights path. The duplication with lines 405-409 is acceptable given the small footprint and independent entry points.
src/AiDotNet.Tensors/Engines/CpuEngine.cs (9)
1422-1432: Type-specialized matmul dispatch looks good (float/double).The NET6+ fast-path routing is clean, and the generic fallback remains intact.
1454-1547: Matmul tiling + row-block parallelization appears race-free.Partitioning by row blocks (
iStart..iEnd) and writing disjointcslices is the right shape for correctness. TheCpuMatMulSingleThreadguard is also handy for diagnostics.
1721-1759: Span-based matrix ops are a nice win (less loop noise, more SIMD).
MatrixAdd,MatrixMultiplyScalar,MatrixSubtract, andMatrixSumOfSquaresmoving tonumOps.*is a clear readability + perf improvement.
1792-1814: OuterProduct + row get/set span copies look good.The row-parallel outer product writes disjoint rows, and
numOps.Copy(...)forGetRow/SetRowis cleaner than element loops.Also applies to: 1842-1846, 1880-1882
2204-2214: Division-by-zero pre-scan: confirm intended behavior for float/double.This now throws on any exact-zero divisor for all
T. That’s consistent with your vector divide behavior earlier in the file, but it is a semantic choice (IEEE floats would normally return ±Inf/NaN). Worth double-checking this matches library expectations.
3001-3002:TensorSum/TensorMinValue/TensorMaxValuechanges look solid.Moving sum to
numOps.Sum(span)and using chunked parallel min/max with per-chunk reductions reads well and should scale better for large tensors.Also applies to: 3068-3105, 3114-3152
12228-12231: Dot-based sum-of-squares + scalar span ops: LGTM.
numOps.Dot(x, x)for sum-of-squares andAddScalar/SubtractScalar/DivideScalarvia spans are consistent with the rest of the SIMD/Span direction.Also applies to: 13305-13330
1664-1683: Verify thread-safety ofnumOps.DotinParallel.For.This code shares one
numOpsinstance across parallel threads. Confirm whetherINumericOperations<T>and itsDotimplementation are documented as thread-safe and stateless. If the implementation maintains any mutable state (caches, scratch buffers, or thread-local fields), this risks race conditions. If thread-safety is not guaranteed, useThreadLocal<INumericOperations<T>>or fall back to sequential execution.
2030-2031: Potential correctness risk: in-place span operations inTensorAddMany/TensorMultiplyManymay corrupt results ifINumericOperations<T>doesn't support destination aliasing.The methods use in-place accumulation like
numOps.Add(resultSpan, tensors[t].AsSpan(), resultSpan)where the destination aliases an input. This is only safe if theINumericOperations<T>implementation explicitly supports span aliasing. SIMD-based implementations usingVector<T>or intrinsics can produce corrupted results with overlapping source/destination spans without overlap detection or direction-aware loops.Document the aliasing contract for
INumericOperations<T>.AddandMultiply, or route in-place operations through a temporary buffer to guarantee correctness.Applies to: 2030–2031, 2094–2103, 2164–2172.
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (4)
1857-1864: New public APIGemmDoubleBuffered: verify kernel name exists in the compiled kernel set.
Since this is now callable externally/tests may rely on it, a missing kernel name would become a runtimeKeyNotFoundException.
1776-1815: Verify safe/unsafe kernel selection heuristics against design specifications and benchmarks.The 512-dimension cutoff for routing between
gemm_medium_tile(safe) andgemm_double_buffered(unsafe) kernels warrants validation. Confirm that:
- This threshold matches your intended behavior and performance profile
- The policy is applied consistently across all GEMM fallback paths
- Typical transformer workload shapes (e.g., 2048×4096, 4096×8192) route to the expected kernel
872-940: Baseline config normalization: verifyNormalizeRowMajorConfigsemantics and database impact.
The code now normalizes CLBlast baseline configs by callingNormalizeRowMajorConfig()on both database-provided and default baselines. This is applied before any kernel selection logic. Confirm what fieldsNormalizeRowMajorConfig()modifies—specifically whether it only normalizesUseColumnMajorA, or whether other layout-related fields are also affected. Additionally, verify whether existing CLBlast database baselines rely onUseColumnMajorA=true, as silent normalization could unintentionally change behavior for affected devices.
943-972:NormalizeRowMajorConfig: verify handling ofGemmConfigfield evolution.This method reconstructs a
GemmConfigwith manual field copying. IfGemmConfigis later extended with new fields, those will silently default in normalized configs rather than inheriting from the input, potentially causing performance or correctness issues.Action needed: Confirm whether
GemmConfigis defined as arecord struct(enablingwith-expression support). If so, consider usingconfig with { UseColumnMajorA = false }to safely handle future field additions. IfGemmConfigis a mutable struct, direct mutation may be preferable to reconstruction.tests/AiDotNet.Tests/DirectGpuTests.cs (1)
650-656: LGTM!Good addition of the
gemm_double_bufferedkernel to the correctness validation suite. This ensures the new double-buffered GEMM path is validated against the reference implementation alongside the other kernel variants.tests/AiDotNet.Tensors.Benchmarks/Program.cs (2)
32-42: LGTM!The new CLI options follow the established pattern and are placed appropriately before the
NET462conditional block. Good integration of the new diagnostic utilities.
78-79: LGTM!Usage text is clear and consistent with the existing help output format.
tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs (2)
12-80: LGTM on overall structure.The diagnostic utility is well-structured with clear separation between correctness validation (safe/unsafe modes) and performance benchmarking. The warmup pass at line 60 before timed iterations is good practice.
22-24: VerifyCpuEnginedisposal requirements.If
CpuEngineimplementsIDisposable, the instance created at line 23 should be wrapped in ausingstatement to ensure proper resource cleanup. Additionally, review theComparemethod to verify that matrices created during comparisons are properly disposed.tests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cs (3)
20-20: Same disposal concern as GPU diagnostics.If
CpuEngineimplementsIDisposable, wrap it in ausingstatement.
61-79: LGTM on environment variable parsing.Good flexibility allowing custom sizes via
AIDOTNET_CPU_MATMUL_SIZESwith sensible defaults. The parsing handles multiple delimiter types which is user-friendly.
38-50: LGTM on benchmark loop structure.The checksum accumulation at line 48 is a good pattern to prevent the compiler from eliminating the multiplication as dead code. Adaptive iteration count based on matrix size is also sensible.
There was a problem hiding this comment.
Pull request overview
This pull request focuses on improving GEMM (General Matrix Multiply) correctness and performance by changing the default OpenCL GEMM behavior to use built-in kernels instead of dynamic/CLBlast kernels (now opt-in), expanding CPU SIMD usage for matrix and tensor operations, and adding diagnostic benchmarks for both CPU and GPU matrix multiplication.
Changes:
- Default OpenCL GEMM to built-in kernels with dynamic kernels as opt-in via environment variable
- Optimize CPU matrix/tensor operations using SIMD vectorization through span-based operations
- Add CPU and GPU matmul diagnostic tools with correctness checking and performance profiling
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AiDotNet.Tests/DirectGpuTests.cs | Adds gemm_double_buffered kernel to correctness tests |
| tests/AiDotNet.Tensors.Benchmarks/Program.cs | Adds command-line options for CPU and GPU matmul diagnostics |
| tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs | New GPU diagnostics with correctness and performance tests |
| tests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cs | New CPU diagnostics with configurable size testing |
| src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs | Changes GEMM default to built-in kernels, adds config normalization, exposes GemmDoubleBuffered |
| src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs | Adds optional GEMM validation for non-finite values |
| src/AiDotNet.Tensors/Engines/CpuEngine.cs | Replaces element-wise loops with SIMD-optimized span operations for matrices and tensors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs (1)
415-429: Consider SIMD optimization forAllFinitecheck.Given the PR's focus on expanding CPU SIMD operations, this validation loop over potentially large GEMM results could benefit from vectorization. However, since this is an opt-in diagnostic feature (
GemmValidateEnabled), the simpler scalar implementation is acceptable.♻️ Optional SIMD-accelerated implementation
private static bool AllFinite(float[] data, out int badIndex) { + int i = 0; + if (System.Numerics.Vector.IsHardwareAccelerated && data.Length >= System.Numerics.Vector<float>.Count) + { + int vectorSize = System.Numerics.Vector<float>.Count; + int vectorEnd = data.Length - (data.Length % vectorSize); + for (; i < vectorEnd; i += vectorSize) + { + var vec = new System.Numerics.Vector<float>(data, i); + // NaN and Infinity fail the equality check with themselves or produce false comparisons + if (System.Numerics.Vector.EqualsAny(vec, vec) == false || + System.Numerics.Vector.GreaterThanOrEqualAll(vec, new System.Numerics.Vector<float>(float.NegativeInfinity)) == false) + { + // Fall back to scalar to find exact index + for (int j = i; j < i + vectorSize; j++) + { + if (float.IsNaN(data[j]) || float.IsInfinity(data[j])) + { + badIndex = j; + return false; + } + } + } + } + } + - for (int i = 0; i < data.Length; i++) + for (; i < data.Length; i++) { float value = data[i]; if (float.IsNaN(value) || float.IsInfinity(value)) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
1636-1638: Packed dynamic GEMM: hard-codeduseColumnMajorC = falsemakes half the method dead; also consider early_dynamicGemmnull check.
Right nowuseColumnMajorCis always false, so the column-major-C branches are unreachable. If that’s intentional, a short comment explaining “output is always row-major in this backend” would prevent future confusion. Also,TryExecutePackedDynamicGemmcan allocate padding buffers before failing viaTryExecuteDynamicGemmwhen_dynamicGemm == null—an early guard would avoid wasted work.Also applies to: 1657-1661
1749-1774: Env var parsing inconsistency: prefer GetEnvBool for dynamic gating + trace.
New knobs (AIDOTNET_GEMM_ENABLE_DYNAMIC) are parsed with== "1"while you already haveGetEnvBoolthat supports common truthy values. UsingGetEnvBoolhere would make behavior consistent across the file (and across platforms / shells).Also applies to: 1776-1789
src/AiDotNet.Tensors/Engines/CpuEngine.cs (5)
37-43: AvoidConsole.WriteLine-style tracing behavior in library code (even when env-gated).Env-gated tracing is useful, but writing to stdout can break consumers (bench harnesses, apps, tests). Prefer
System.Diagnostics.Trace(or an injected logger) so output can be routed/filtered.Proposed tweak
- private static readonly bool CpuMatMulTraceEnabled = - Environment.GetEnvironmentVariable("AIDOTNET_CPU_MATMUL_TRACE") == "1"; + private static readonly bool CpuMatMulTraceEnabled = + Environment.GetEnvironmentVariable("AIDOTNET_CPU_MATMUL_TRACE") == "1"; ... - Console.WriteLine($"[CpuMatMul] float {m}x{k}x{n} tile={tileSize} parallel={useParallel}"); + System.Diagnostics.Trace.WriteLine($"[CpuMatMul] float {m}x{k}x{n} tile={tileSize} parallel={useParallel}");
1422-1434:Unsafe.Asreturn casting: correct but consider simpler/safer expression.This is correct given the
typeof(T) == typeof(float/double)guards, but it’s still “sharp”; a simple(Matrix<T>)(object)resultis easier to reason about and debug.Possible simplification
- var result = MatrixMultiplyFloat(floatA, floatB); - return Unsafe.As<Matrix<float>, Matrix<T>>(ref result); + var result = MatrixMultiplyFloat(floatA, floatB); + return (Matrix<T>)(object)result;
1456-1550: Tiled/parallel matmul path looks race-free; verifyMatrix<T>span layout + consider avoiding repeated span retrieval inside the parallel body.Partitioning by
iblocks ensures disjointcwrites, so races shouldn’t occur. Minor perf nit: insideParallel.For, re-callinga.AsSpan()/b.AsSpan()/result.AsWritableSpan()every block is redundant.
1551-1573: Parallelization threshold math: good overflow handling; consider making the threshold configurable for benchmarking.
Math.BigMul+ saturation avoids overflow. If diagnostics/benchmarks are a goal, letting the threshold be env-configurable (similar to tile size) can help tune without code changes.
1670-1689:MatrixVectorMultiply: parallel branch is OK; avoid per-iteration span wrapper allocation if possible.Inside the parallel loop you create
new ReadOnlySpan<T>(vectorData)each row. It’s small but measurable at scale; consider lifting immutable data outside the loop if you can do so without capturing spans across threads (e.g., usevectorDatadirectly in a dedicated dot implementation that accepts arrays + offset).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/Program.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tensors.Benchmarks/Program.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tensors.Benchmarks/Program.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
🔇 Additional comments (25)
src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs (3)
38-39: LGTM!The opt-in validation flag via environment variable is a sensible approach for diagnostic features. The static readonly initialization ensures the check is performed once at startup.
405-409: LGTM!The validation check is appropriately placed after downloading GPU results and before type conversion. Returning
nullto trigger CPU fallback is consistent with the existing fallback pattern in this class.
444-448: LGTM!The validation is consistent with the
MatMul<T>method, maintaining the same fallback behavior for cached weight operations.src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
901-907: Baseline/default config normalization looks right; consider removing redundancy + ensure KernelName semantics still match.
Normalizing the CLBlast database baseline and the fallback default to “row-major A” is a good correctness guard. ThedefaultBaseline = NormalizeRowMajorConfig(defaultBaseline);call is redundant now thatUseColumnMajorA = falseis set explicitly, but harmless.Also applies to: 912-940
1857-1864: New public APIGemmDoubleBuffered: verify it’s intended as part of the supported surface.
Looks fine mechanically (routes throughExecuteGemmKernel("gemm_double_buffered", ...)). Just ensure this method is meant to be public/stable vs an internal tuning hook (since it becomes part of the class’ API contract).
4459-4464: Help text advertisesAIDOTNET_GEMM_VALIDATE, but this file doesn't implement it.
In this file,AIDOTNET_GEMM_VALIDATEonly appears inPrintDiagnosticHelp()—there's no corresponding validation path inGemm(...). Either wire it up (if intended here) or remove/clarify the help entry to avoid a misleading knob.tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs (6)
1-11: LGTM!Imports and class declaration are appropriate for the diagnostics functionality.
82-91: LGTM!Efficient random matrix generation using
AsWritableSpanwith values uniformly distributed in [-1, 1].
93-128: LGTM!The comparison logic correctly handles non-finite GPU values by counting them separately and computing error statistics only on valid elements. Using
doublefor error accumulation is appropriate.
130-135: LGTM!Clean output formatting with appropriate precision specifiers.
137-141: Environment variable changes at runtime may not affect engine behavior.If the GPU engine reads
AIDOTNET_GEMM_SAFE/AIDOTNET_GEMM_UNSAFEonly during initialization (common pattern), callingSetKernelModeafter line 22 (AiDotNetEngine.Current) won't switch kernels. Both "safe" and "unsafe" correctness checks would then use the same code path.Run the following to see when/how these env vars are consumed:
#!/bin/bash # Find where AIDOTNET_GEMM env vars are read rg -n -C5 'AIDOTNET_GEMM' --type=cs
64-71: Verify ifMatrixMultiplyrequires explicit GPU synchronization for accurate timing.Host
Stopwatchtiming may underreport actual GPU kernel execution time ifMatrixMultiplyis asynchronous. Confirm whether the GPU engine handles synchronization internally or if explicit synchronization (fence, event wait, or device sync) is needed after the multiply before stopping the timer.tests/AiDotNet.Tensors.Benchmarks/Program.cs (2)
32-42: LGTM!The new CLI options follow the existing dispatch pattern and integrate cleanly with the rest of the argument handling.
78-79: LGTM!Usage text is clear and consistent with existing option descriptions.
src/AiDotNet.Tensors/Engines/CpuEngine.cs (11)
1575-1651: SIMD inner-loop block multiply: indexing looks correct; ensureSimdVector.MatMulInnerLoop*contract matches the slice semantics.You’re slicing
b/cto[j0..jEnd)and passing0..(jEnd-j0)to the SIMD helper. This is only correct if the helper interprets the provided spans as already-offset (i.e., it must not apply additional base indexing assumptions).
1727-1766: Span-based matrix ops (Add,MultiplyScalar,Subtract,SumOfSquares) look good; confirm span lengths are always identical for allMatrix<T>implementations.Assuming
Matrix<T>.AsSpan()is a tight row-major buffer, these are a clear win.
1798-1820:OuterProductparallelization is fine; validateGetRowSpan(i)returns disjoint writable memory.The parallel write pattern assumes each row span is independent and points into the backing buffer at non-overlapping ranges.
1848-1852: Row get/set span copies: nice cleanup.Using
numOps.Copyshould be faster and clearer than manual loops, assumingCopyis optimized.Also applies to: 1886-1888
2036-2037:TensorAddspan path: LGTM.Cleaner and likely faster than per-element loops.
2210-2221:TensorDividenow does explicit divide-by-zero checks: correctness win.This is a good trade-off for predictable behavior (especially for non-float types where IEEE semantics aren’t expected).
2554-2571: ElementwiseTensorPower(bases, exponents)now writes via backing arrays: good; ensure.Datamatches.Lengthand is the authoritative storage.This assumes
Tensor<T>.Datais the canonical contiguous storage for the tensor and not a view with indirection.
3009-3113:TensorSum+TensorMaxValuerefactor: good use of span reducers; parallel max reduction looks correct.The chunking logic avoids empty spans and the final combine honors “first value wins” semantics without depending on
MinValue.
3128-3160:TensorMinValuerefactor mirrors max path; looks correct.Same notes as max path.
12237-12239:TensorSumOfSquaresusing dot with itself: good optimization.As long as
Dot(x, x)is implemented as sum(x[i]*x[i]) (and not e.g. fused-with-sqrt), this is ideal.
13313-13338: Scalar ops switched to span intrinsics: looks good.These should reduce overhead and centralize SIMD opportunities in
INumericOperations<T>.
…operations and switch to Trace logging
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/AiDotNet.Tensors/LinearAlgebra/MatrixBase.cs (1)
57-77: Critical: internal ctor allows “extra elements” to participate in math (data.Length > rows*cols).Because many operations span over the full
_dataarray, permittingdata.Length != rows*colscan silently corrupt results. Also, this ctor skips the non-negative checks and doesn’t guardrows * colsoverflow.Proposed fix (enforce invariants + overflow-safe required length)
internal MatrixBase(int rows, int cols, T[] data) { - _rows = rows; - _cols = cols; - _data = data ?? throw new ArgumentNullException(nameof(data)); - if (data.Length < rows * cols) - throw new ArgumentException("Data array is too small for the specified dimensions."); + if (rows < 0) throw new ArgumentException("Rows must be non-negative", nameof(rows)); + if (cols < 0) throw new ArgumentException("Columns must be non-negative", nameof(cols)); + + _data = data ?? throw new ArgumentNullException(nameof(data)); + + int required = checked(rows * cols); + if (_data.Length != required) + throw new ArgumentException($"Data array length must be exactly {required} for the specified dimensions.", nameof(data)); + + _rows = rows; + _cols = cols; }src/AiDotNet.Tensors/Engines/DirectGpu/GemmBenchmark.cs (1)
125-154: Trace output may be invisible in console runs without configured listeners.If users run
GemmBenchmark.QuickTest()from a console without configuringTrace.Listeners, they may see no output. Consider either (a) ensuring benchmark/test hosts configureTextWriterTraceListener(Console.Out)withTrace.AutoFlush = true, or (b) keeping QuickTest output onConsole.WriteLinefor immediate visibility.Also applies to: 202-237
src/AiDotNet.Tensors/Engines/CpuEngine.cs (1)
12-49: Fix duplicated XML doc tags (<summary>/<remarks>) onCpuEngineYou now have two
<summary>blocks and two<remarks>blocks in the same doc comment, which can produce invalid XML docs / warnings-as-errors in doc builds.Proposed fix (keep the newer “For Beginners” wording, remove the duplicate block)
@@ -/// <summary> -/// CPU-based execution engine using INumericOperations for type-generic operations. -/// </summary> -/// <remarks> -/// <para> -/// CpuEngine provides the default execution backend for AiDotNet. It works with -/// any numeric type that implements INumericOperations{T}, including decimal, -/// BigInteger, and custom numeric types. -/// </para> -/// <para><b>For Beginners:</b> This is the standard, "always works" mode. -/// -/// CpuEngine characteristics: -/// - Works with ANY numeric type (float, double, decimal, BigInteger, custom types) -/// - No special hardware required -/// - Good performance for small-to-medium datasets -/// - Single-threaded by default (can be parallelized in future versions) -/// -/// When to use: -/// - You need decimal or high-precision arithmetic -/// - You don't have a GPU -/// - Your datasets are small (< 100K parameters) -/// - You're using custom numeric types -/// </para> -/// </remarks> /// <summary> /// CPU-based execution engine using INumericOperations for type-generic operations. /// </summary> /// <remarks> @@ /// and works on every computer without any extra setup.</para> /// </remarks> public class CpuEngine : IEnginesrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (1)
685-718:SimpleConsoleLoggershouldn’t write viaTrace.WriteLine(likely loses output).
This type is named “ConsoleLogger” and changesConsole.ForegroundColor, but it emits viaTrace.WriteLine(Line 711/716). If noTraceListeneris configured, users enabling tuning diagnostics may see nothing.Proposed diff
if (color.HasValue) { var previous = Console.ForegroundColor; Console.ForegroundColor = color.Value; - Trace.WriteLine(message); + Console.WriteLine(message); Console.ForegroundColor = previous; } else { - Trace.WriteLine(message); + Console.WriteLine(message); }
🧹 Nitpick comments (20)
src/AiDotNet.Tensors/NumericOperations/HalfOperations.cs (1)
249-263: Consider usingHalf.IsFinitefor clarity.The condition
Half.IsNaN(x[i]) || Half.IsInfinity(x[i])is correct but could be simplified to!Half.IsFinite(x[i]), which more directly expresses the intent and aligns with the method nameAllFinite.Also, the comment on line 251 mentions "Fallback" but there's no SIMD fast-path preceding it—consider removing or updating the comment.
Suggested diff
public bool AllFinite(ReadOnlySpan<Half> x, out int badIndex) { - // Fallback or find the exact index of the non-finite value for (int i = 0; i < x.Length; i++) { - if (Half.IsNaN(x[i]) || Half.IsInfinity(x[i])) + if (!Half.IsFinite(x[i])) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/NumericOperations/FloatOperations.cs (1)
868-900: Finiteness scan is correct; considerfloat.IsFinitefor clarity (if available).
Current logic is correct and returns the first failing index. If your TFMs allow it,!float.IsFinite(x[i])simplifies the condition.Proposed refactor (if `float.IsFinite` is available in your target frameworks)
public bool AllFinite(ReadOnlySpan<float> x, out int badIndex) { // Fallback or find the exact index of the non-finite value for (int i = 0; i < x.Length; i++) { - if (float.IsNaN(x[i]) || float.IsInfinity(x[i])) + if (!float.IsFinite(x[i])) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/NumericOperations/DoubleOperations.cs (1)
804-836: Finiteness scan is correct; considerdouble.IsFinitefor clarity (if available).
Current logic is correct and returns the first failing index. If your TFMs allow it,!double.IsFinite(x[i])simplifies the condition.Proposed refactor (if `double.IsFinite` is available in your target frameworks)
public bool AllFinite(ReadOnlySpan<double> x, out int badIndex) { // Fallback or find the exact index of the non-finite value for (int i = 0; i < x.Length; i++) { - if (double.IsNaN(x[i]) || double.IsInfinity(x[i])) + if (!double.IsFinite(x[i])) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/NumericOperations/ComplexOperations.cs (1)
963-988: LGTM!The implementation correctly checks both real and imaginary parts for NaN/Infinity values. The pattern is consistent with the interface contract.
For consistency with
MultivectorOperations(which reuses itsIsNaN/IsInfinitymethods), consider refactoring to reuse the existing instance methods:♻️ Optional refactor for consistency
public bool AllFinite(ReadOnlySpan<Complex<T>> x, out int badIndex) { for (int i = 0; i < x.Length; i++) { - if (_ops.IsNaN(x[i].Real) || _ops.IsInfinity(x[i].Real) || - _ops.IsNaN(x[i].Imaginary) || _ops.IsInfinity(x[i].Imaginary)) + if (IsNaN(x[i]) || IsInfinity(x[i])) { badIndex = i; return false; } } badIndex = -1; return true; }src/AiDotNet.Tensors/Engines/DirectGpuTensorEngine.cs (1)
797-816: Zero-copyMatrix<T>construction: ensureresultDatasize + ownership assumptions holdThis is a nice win, but it now implicitly relies on
resultDatabeing (1) correctly sized (Rows * Cols) and (2) not reused/mutated elsewhere (since the matrix will alias it).Proposed defensive guard (cheap correctness check)
var resultData = _directGpu.MatMul(a.AsSpan().ToArray(), b.AsSpan().ToArray(), a.Rows, a.Columns, b.Columns); if (resultData == null) return base.MatrixMultiply(a, b); - return new Matrix<T>(a.Rows, b.Columns, resultData); + if (resultData.Length != a.Rows * b.Columns) + return base.MatrixMultiply(a, b); + + return new Matrix<T>(a.Rows, b.Columns, resultData);To verify the invariants quickly, I’d grep
DirectGpuEngine.MatMulto confirm it always returns a freshT[]with lengthM*N(and row-major order matchingMatrix<T>expectations).src/AiDotNet.Tensors/Engines/DirectGpu/Profiling/GemmProfiler.cs (1)
31-32: Update comment to reflect Trace output.The comment states "print progress to console" but the implementation now uses
Trace.WriteLine. Consider updating the documentation for accuracy.- /// <summary>Whether to print progress to console.</summary> + /// <summary>Whether to write profiling progress to trace output.</summary> public bool Verbose { get; init; } = true;src/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipBackend.cs (4)
5-8:using System.Diagnosticsaddition is correct and unblocksTrace.WriteLine.
No issues with the import itself. Consider also de-qualifyingSystem.Diagnostics.Debug.WriteLineusages for consistency now thatSystem.Diagnosticsis in scope.
205-214: Good: “no GEMM kernels compiled” is now routed viaTraceinstead of stdout.
Minor improvement: include_architectureand/orDeviceNamein the message to reduce “why did it fail?” follow-ups.
259-358: Consider honoringEnableDiagnostics(and/or a TraceSwitch) to avoid always-on noisy tracing.
Right now these traces emit even whenHipNativeBindings.EnableDiagnostics == false, which makes the toggle less meaningful and can spam logs in normal runs.Proposed change (gate Trace logging behind EnableDiagnostics)
public sealed class HipBackend : IAsyncGpuBackend { + private static void TraceDiag(string message) + { + if (!EnableDiagnostics) return; + Trace.WriteLine(message); + } + private IntPtr _stream; @@ - Trace.WriteLine($"[HipBackend] Compiling kernels for {_architecture} with flags: {compileFlags}"); + TraceDiag($"[HipBackend] Compiling kernels for {_architecture} with flags: {compileFlags}"); @@ - Trace.WriteLine($"[HipBackend] Kernel compilation complete. Available kernels: {_kernelCache.Count}"); + TraceDiag($"[HipBackend] Kernel compilation complete. Available kernels: {_kernelCache.Count}"); @@ - Trace.WriteLine($"[HipBackend] Kernel compilation EXCEPTION: {ex.GetType().Name}: {ex.Message}"); + TraceDiag($"[HipBackend] Kernel compilation EXCEPTION: {ex.GetType().Name}: {ex.Message}");
365-430: Trace logs are helpful here; consider using severity + the same gating helper.
These are effectively error paths (hiprtcCreateProgram, compile log,hipModuleLoadData). UsingTrace.TraceError/Trace.TraceWarning(and gating viaEnableDiagnostics) makes downstream log filtering easier.tests/AiDotNet.Tensors.Benchmarks/Helpers/BenchmarkHelper.cs (1)
64-78: Consider checking reference values for non-finite as well.The comparison logic correctly detects non-finite values in
actual, but ifrefSpan[i]happens to be NaN or Infinity,Math.Abs(refSpan[i] - actVal)would produce NaN, silently corruptingmaxErrorandsumError.Since
CreateRandomMatrixproduces finite values, this is unlikely in practice. However, for a general-purpose comparison utility, defensive handling would improve robustness.♻️ Optional: Check both spans for non-finite values
for (int i = 0; i < actSpan.Length; i++) { float actVal = actSpan[i]; - if (float.IsNaN(actVal) || float.IsInfinity(actVal)) + float refVal = refSpan[i]; + if (!float.IsFinite(actVal) || !float.IsFinite(refVal)) { nonFiniteCount++; continue; } - double error = Math.Abs(refSpan[i] - actVal); + double error = Math.Abs(refVal - actVal); sumError += error; if (error > maxError) maxError = error; count++; }tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs (1)
68-68: Consider exception-safe kernel mode reset.If an exception occurs during the correctness tests (lines 47-66), the kernel mode environment variables won't be reset to defaults. For robustness, consider wrapping the correctness block in a
try/finally.♻️ Suggested improvement
+ try + { foreach (int size in correctnessSizes) { // ... existing correctness test code ... } + } + finally + { SetKernelMode(forceSafe: false, forceUnsafe: false); + }src/AiDotNet.Tensors/Engines/CpuEngine.cs (4)
52-59: Env-var toggles: consider accepting more truthy values (optional)
== "1"is fine, but supporting"true"/"TRUE"(and trimming) makes local debugging less brittle.
1440-1451: AvoidUnsafe.Asfor matmul fast-path return; use a normal cast instead
Unsafe.As<Matrix<float>, Matrix<T>>(ref result)is non-idiomatic and easy to simplify, while keeping the same behavior whenTis actuallyfloat/double. Also avoids accidental misuse if this code is later refactored.Proposed fix
@@ #if NET6_0_OR_GREATER if (typeof(T) == typeof(float) && a is Matrix<float> floatA && b is Matrix<float> floatB) { - var result = MatrixMultiplyFloat(floatA, floatB); - return Unsafe.As<Matrix<float>, Matrix<T>>(ref result); + return (Matrix<T>)(object)MatrixMultiplyFloat(floatA, floatB); } if (typeof(T) == typeof(double) && a is Matrix<double> doubleA && b is Matrix<double> doubleB) { - var result = MatrixMultiplyDouble(doubleA, doubleB); - return Unsafe.As<Matrix<double>, Matrix<T>>(ref result); + return (Matrix<T>)(object)MatrixMultiplyDouble(doubleA, doubleB); } #endif
1688-1707: ParallelMatrixVectorMultiply: confirmnumOps.Dotis thread-safe; small perf tidy-upThis parallelizes correctly by row. Please confirm
INumericOperations<T>implementations are stateless/thread-safe (sharednumOpsinstance across threads).Optional: hoist
ReadOnlySpan<T> vectorSpan = vector.Data;outside the lambda to avoid re-creating spans per-iteration.
1816-1841: Outer product parallelization looks good; consider reusingbSpan(optional)Correct parallel partitioning (by row). Minor:
var bSpan = b.AsSpan();can be computed once and reused in both branches.src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (4)
16-46: Remove the duplicated XML doc block (and reconsider “For Beginners” in public API docs).
Right now the class has two consecutive<summary>/<remarks>blocks (Line 16–29 and Line 30–46) that repeat the same “Key Features” list, which will bloat/duplicate generated docs and is easy to desync over time.
80-110: Initialize all public device-info properties consistently on the “OpenCL not available” path.
On the early return (Line 156–162)ComputeUnits/GlobalMemoryBytes/LocalMemoryBytesare left as implicit defaults; it’s clearer/safer to set them explicitly alongsideDeviceName/DeviceVendor.Proposed diff
if (!DirectOpenClContext.IsAvailable) { IsAvailable = false; DeviceName = "None"; DeviceVendor = "None"; + ComputeUnits = 0; + GlobalMemoryBytes = 0; + LocalMemoryBytes = 0; return; }Also applies to: 156-189
164-517: Consider gating the heavyTrace.WriteLineusage to avoid noisy/prod overhead.
A lot of the new tracing uses interpolated strings (Line 166+ / kernel compilation block) which allocates even when tracing isn’t consumed. If you want “Trace-level” diagnostics to be opt-in, consider guarding with an env toggle (likeAIDOTNET_GEMM_TRACE) or aTraceSwitch/TraceSource.
1900-1908: Don’t log everyMatMulcall unconditionally.
Trace.WriteLine($"[OpenClBackend.MatMul] Called: ...")(Line 1902) can become very noisy in real workloads. Suggest gating it behindAIDOTNET_GEMM_TRACE(or similar).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
src/AiDotNet.Tensors/Engines/AiDotNetEngine.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/GemmBenchmark.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/DynamicGemmKernel.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/GemmAutoTuner.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/Profiling/GemmProfiler.cssrc/AiDotNet.Tensors/Engines/DirectGpuTensorEngine.cssrc/AiDotNet.Tensors/Engines/Engine.cssrc/AiDotNet.Tensors/Helpers/TensorPrimitivesHelper.cssrc/AiDotNet.Tensors/Interfaces/IVectorizedOperations.cssrc/AiDotNet.Tensors/LinearAlgebra/Matrix.cssrc/AiDotNet.Tensors/LinearAlgebra/MatrixBase.cssrc/AiDotNet.Tensors/NumericOperations/ByteOperations.cssrc/AiDotNet.Tensors/NumericOperations/ComplexOperations.cssrc/AiDotNet.Tensors/NumericOperations/DecimalOperations.cssrc/AiDotNet.Tensors/NumericOperations/DoubleOperations.cssrc/AiDotNet.Tensors/NumericOperations/FloatOperations.cssrc/AiDotNet.Tensors/NumericOperations/HalfOperations.cssrc/AiDotNet.Tensors/NumericOperations/Int32Operations.cssrc/AiDotNet.Tensors/NumericOperations/Int64Operations.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/NumericOperations/SByteOperations.cssrc/AiDotNet.Tensors/NumericOperations/ShortOperations.cssrc/AiDotNet.Tensors/NumericOperations/UInt16Operations.cssrc/AiDotNet.Tensors/NumericOperations/UInt32Operations.cssrc/AiDotNet.Tensors/NumericOperations/UInt64Operations.cssrc/AiDotNet.Tensors/NumericOperations/UIntOperations.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cstests/AiDotNet.Tensors.Benchmarks/Helpers/BenchmarkHelper.cs
✅ Files skipped from review due to trivial changes (1)
- src/AiDotNet.Tensors/Engines/Engine.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Helpers/TensorPrimitivesHelper.cssrc/AiDotNet.Tensors/Engines/DirectGpu/GemmBenchmark.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/Profiling/GemmProfiler.cssrc/AiDotNet.Tensors/LinearAlgebra/MatrixBase.cssrc/AiDotNet.Tensors/LinearAlgebra/Matrix.cssrc/AiDotNet.Tensors/NumericOperations/UIntOperations.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/NumericOperations/FloatOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/DynamicGemmKernel.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/NumericOperations/ShortOperations.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/Engines/AiDotNetEngine.cssrc/AiDotNet.Tensors/NumericOperations/ComplexOperations.cssrc/AiDotNet.Tensors/NumericOperations/ByteOperations.cstests/AiDotNet.Tensors.Benchmarks/Helpers/BenchmarkHelper.cssrc/AiDotNet.Tensors/NumericOperations/Int32Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/GemmAutoTuner.cssrc/AiDotNet.Tensors/NumericOperations/DecimalOperations.cssrc/AiDotNet.Tensors/NumericOperations/Int64Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpuTensorEngine.cssrc/AiDotNet.Tensors/NumericOperations/UInt16Operations.cssrc/AiDotNet.Tensors/NumericOperations/HalfOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipNativeBindings.cssrc/AiDotNet.Tensors/NumericOperations/UInt64Operations.cssrc/AiDotNet.Tensors/NumericOperations/UInt32Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Interfaces/IVectorizedOperations.cssrc/AiDotNet.Tensors/NumericOperations/DoubleOperations.cssrc/AiDotNet.Tensors/NumericOperations/SByteOperations.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Helpers/TensorPrimitivesHelper.cssrc/AiDotNet.Tensors/Engines/DirectGpu/GemmBenchmark.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/Profiling/GemmProfiler.cssrc/AiDotNet.Tensors/LinearAlgebra/MatrixBase.cssrc/AiDotNet.Tensors/LinearAlgebra/Matrix.cssrc/AiDotNet.Tensors/NumericOperations/UIntOperations.cstests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/NumericOperations/FloatOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/DynamicGemmKernel.cstests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cssrc/AiDotNet.Tensors/NumericOperations/ShortOperations.cssrc/AiDotNet.Tensors/NumericOperations/OctonionOperations.cssrc/AiDotNet.Tensors/Engines/AiDotNetEngine.cssrc/AiDotNet.Tensors/NumericOperations/ComplexOperations.cssrc/AiDotNet.Tensors/NumericOperations/ByteOperations.cstests/AiDotNet.Tensors.Benchmarks/Helpers/BenchmarkHelper.cssrc/AiDotNet.Tensors/NumericOperations/Int32Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/GemmAutoTuner.cssrc/AiDotNet.Tensors/NumericOperations/DecimalOperations.cssrc/AiDotNet.Tensors/NumericOperations/Int64Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpuTensorEngine.cssrc/AiDotNet.Tensors/NumericOperations/UInt16Operations.cssrc/AiDotNet.Tensors/NumericOperations/HalfOperations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipNativeBindings.cssrc/AiDotNet.Tensors/NumericOperations/UInt64Operations.cssrc/AiDotNet.Tensors/NumericOperations/UInt32Operations.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Interfaces/IVectorizedOperations.cssrc/AiDotNet.Tensors/NumericOperations/DoubleOperations.cssrc/AiDotNet.Tensors/NumericOperations/SByteOperations.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cssrc/AiDotNet.Tensors/Engines/AiDotNetEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipBackend.cs
🔇 Additional comments (53)
src/AiDotNet.Tensors/NumericOperations/OctonionOperations.cs (1)
318-342: LGTM!The implementation is clean and correct:
AllFiniteproperly leverages the existingIsNaNandIsInfinitymethods which already check all 8 octonion components.- The
badIndexout parameter is correctly set in all code paths.IsAnyNonFiniteappropriately delegates toAllFiniteto avoid code duplication.- The scalar iteration approach is consistent with
SupportsCpuAcceleration => falsefor this type.src/AiDotNet.Tensors/NumericOperations/SByteOperations.cs (1)
741-759: LGTM!The implementations are correct. Since
sbyteis an integral type (range -128 to 127), it cannot represent NaN or Infinity values—all sbyte values are inherently finite. ReturningtrueforAllFiniteandfalseforIsAnyNonFinitewithbadIndex = -1is the appropriate behavior, consistent with the existingIsNaNandIsInfinitymethods that also returnfalsefor this type.src/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipNativeBindings.cs (2)
6-6: LGTM!The
System.Diagnosticsimport is correctly added to support theTrace.WriteLineusage in theLogDiagnosticmethod.
291-295: LGTM!Switching from
Console.WriteLinetoTrace.WriteLineis the correct approach for diagnostic output. This allows consumers to control trace output viaTraceListenerconfiguration without polluting console output in production. The change aligns with the PR-wide pattern of standardizing on Trace-based diagnostics across DirectGpu backends.src/AiDotNet.Tensors/NumericOperations/UInt64Operations.cs (1)
764-782: LGTM!The implementations are correct and consistent with the existing pattern in this class. Since
ulongis an integral type that cannot represent NaN or Infinity,AllFinitecorrectly returnstrueandIsAnyNonFinitecorrectly returnsfalse, both withbadIndex = -1. This aligns with the existingIsNaNandIsInfinitymethods at lines 647 and 669 that also returnfalse.Note: The AI summary mentions duplicate method declarations, but the provided code shows only a single definition of each method, so no duplication issue is present.
src/AiDotNet.Tensors/NumericOperations/UInt16Operations.cs (1)
706-724: LGTM!The implementations are correct. Since
ushortis an integer type that cannot represent NaN or Infinity,AllFinitecorrectly always returnstrueandIsAnyNonFinitecorrectly always returnsfalse, both withbadIndex = -1. This is consistent with the existingIsNaNandIsInfinitymethods in this class.src/AiDotNet.Tensors/NumericOperations/Int64Operations.cs (1)
790-808: LGTM!The implementations are correct for the Int64 type. Since
longvalues cannot represent NaN or Infinity,AllFinitetrivially returnstrueandIsAnyNonFinitetrivially returnsfalse. This is consistent with the existingIsNaNandIsInfinitymethods in this class that also return constant values for integers. The documentation clearly explains the rationale.src/AiDotNet.Tensors/NumericOperations/HalfOperations.cs (1)
265-274: LGTM!Clean delegation to
AllFinitewith proper negation. The XML documentation is thorough and correctly describes the behavior.src/AiDotNet.Tensors/NumericOperations/UInt32Operations.cs (2)
715-723: LGTM!The implementation correctly returns
truefor all UInt32 spans since integral types cannot represent NaN or Infinity. ThebadIndex = -1convention is appropriate.
725-733: LGTM!Correctly returns
falsesince UInt32 values are always finite by definition. The implementation is consistent withAllFiniteand follows the expected interface contract.src/AiDotNet.Tensors/NumericOperations/Int32Operations.cs (2)
743-761: LGTM!The
AllFiniteandIsAnyNonFiniteimplementations are correct. SinceInt32has no representation for NaN or Infinity (consistent with the existingIsNaNandIsInfinitymethods), all integer values are inherently finite. Returning constant results without iterating the span is the appropriate optimization.
763-764: I cannot proceed without a review comment to rewrite. Please provide the review comment enclosed in<review_comment>tags along with any relevant code context or verification results.src/AiDotNet.Tensors/NumericOperations/DecimalOperations.cs (1)
694-712: Constant finiteness results fordecimalare correct and efficient.
Returningtrue/falsewithbadIndex = -1matchesdecimalsemantics and avoids unnecessary scans.src/AiDotNet.Tensors/Helpers/TensorPrimitivesHelper.cs (1)
351-360: VerifyVector<T>.AsSpan()is a safe, non-copying view.
This helper’s correctness/perf hinges onx.AsSpan()returning a stable span over the vector’s backing storage (not a temporary/copy).src/AiDotNet.Tensors/NumericOperations/UIntOperations.cs (1)
725-744: Constant finiteness results foruintlook good.
These implementations are correct, fast, and consistent with the new interface surface.src/AiDotNet.Tensors/NumericOperations/ByteOperations.cs (1)
674-693: Constant finiteness results forbytelook good.
These implementations are correct, fast, and consistent with the new interface surface.src/AiDotNet.Tensors/NumericOperations/MultivectorOperations.cs (1)
318-342: LGTM!The
AllFiniteandIsAnyNonFiniteimplementations are correct and cleanly reuse the existingIsNaNandIsInfinitymethods. The pattern of delegatingIsAnyNonFinitetoAllFinitewith inverted result is consistent with other numeric operation classes.src/AiDotNet.Tensors/NumericOperations/ShortOperations.cs (1)
702-720: LGTM!The implementation correctly returns fixed values since
shortis an integer type that cannot represent NaN or Infinity. The documentation accurately reflects this behavior, and the pattern is consistent with the existingIsNaNandIsInfinitymethods in this class.src/AiDotNet.Tensors/Engines/DirectGpu/Profiling/GemmProfiler.cs (3)
138-146: LGTM!The transition from
Console.WriteLinetoTrace.WriteLineis appropriate for library code. This allows consumers to configure trace listeners and control diagnostic output without polluting stdout.
161-167: Error handling preserved correctly.The exception handling continues profiling other sizes after an error, which is appropriate for a benchmarking tool. The Trace output maintains the same error information as before.
316-341: Consistent logging changes.The rectangular profiling method follows the same Trace.WriteLine pattern as
RunFullProfile, maintaining consistency across the codebase.src/AiDotNet.Tensors/LinearAlgebra/Matrix.cs (1)
24-33: Verify MatrixBase invariant enforcement and all internal caller compliance. Ensure the internalMatrix(int rows, int cols, T[] data)constructor only receives exact-length backing arrays and thatMatrixBase(int,int,T[])enforces overflow and length validation.src/AiDotNet.Tensors/Engines/AiDotNetEngine.cs (2)
1-4: LGTM on using directives and logging migration.The switch from
Console.WriteLinetoTrace.WriteLinealigns with the PR's goal of enabling Trace-based diagnostics across the codebase. This allows diagnostic output to be captured by trace listeners or suppressed in production without code changes.
157-161: LGTM on the newGetEngineInfo()method.Clean utility method that exposes engine name and GPU support status. This complements the existing engine configuration API and supports the new diagnostics infrastructure.
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/DynamicGemmKernel.cs (2)
236-266: LGTM onLogDiagTrace migration.The fallback path in
LogDiagnow correctly usesTrace.WriteLineinstead ofConsole.WriteLine. This maintains the existing file-logging priority while providing consistent trace-based output when file logging fails or is not configured.
374-384: LGTM on kernel compilation failure diagnostics.The kernel source output on compilation failure now routes through
Trace.WriteLine, which is appropriate for diagnostic tooling. This allows capture via trace listeners during debugging without polluting console output in production.tests/AiDotNet.Tensors.Benchmarks/Helpers/BenchmarkHelper.cs (1)
28-38: LGTM onCreateRandomMatrix.Clean implementation using the writable span API for efficient matrix initialization. The [-1, 1] range is well-suited for benchmark scenarios involving neural network weights.
tests/AiDotNet.Tensors.Benchmarks/CpuMatMulDiagnostics.cs (2)
30-76: LGTM on benchmark structure.The benchmark implementation follows good practices:
- Seeded random for reproducibility
- Warmup phase before timing
- Reduced iterations for larger matrices to keep runtime reasonable
- GFLOPS calculation correctly uses 2N³ for matrix multiplication
- Checksum prevents dead-code elimination by optimizers
37-38: Verify ifCpuEnginerequires disposal.
CpuEngineis instantiated at line 37 but never disposed. IfCpuEngineimplementsIDisposable, consider wrapping it in ausingstatement or callingDispose()at the end ofRun().src/AiDotNet.Tensors/Engines/DirectGpu/DirectGpuEngine.cs (4)
32-33: LGTM onGemmValidateEnabledconfiguration.Environment variable-driven opt-in validation is a clean approach. Disabled by default avoids performance overhead in production while enabling correctness debugging when needed.
426-430: LGTM on validation inMatMulWithCachedWeights.Consistent application of the non-finite validation check in the cached weights path ensures GPU correctness validation covers both GEMM entry points.
94-122: LGTM on constructor logging migration.The initialization flow now uses
Trace.WriteLineconsistently, aligning with the broader diagnostic infrastructure changes. The detailed logging of backend discovery attempts aids debugging without cluttering console output.
397-411: Unable to verify GPU GEMM validation logic—manual code review required.The review comment asserts that
IsAnyNonFinitedelegates to the numeric operations API and maintains consistency with finite-check infrastructure. However, I cannot access the repository to confirm:
- Whether the
IsAnyNonFinitemethod exists on the expected interface- Whether the delegation pattern is correctly implemented
- Whether the numeric operations API usage is consistent with the PR's changes
Please manually verify these claims against the codebase.
tests/AiDotNet.Tensors.Benchmarks/GpuMatMulDiagnostics.cs (8)
1-10: LGTM!Using statements and namespace declaration are appropriate for the benchmark context.
11-20: LGTM!Clear documentation and appropriate class design for a diagnostic utility.
47-66: LGTM!The correctness test loop structure is sound. Testing both safe and unsafe kernel modes with the size restriction for unsafe mode (≤512) is a reasonable approach for diagnostics.
70-98: LGTM!The performance measurement approach is well-structured: warmup pass to avoid cold-start artifacts, per-iteration timing with
Stopwatch, and correct GFLOPS calculation (2 × N³operations for matrix multiply).
113-118: LGTM!Clean formatting helper with appropriate scientific notation for error metrics.
120-124: Environment variable manipulation is process-global.This is acceptable for a single-threaded diagnostic tool, but be aware that
Environment.SetEnvironmentVariableaffects the entire process. If these diagnostics are ever run in parallel or integrated into a test suite with concurrent tests, this could cause race conditions.
101-111: Verify ifMatrix<float>results require disposal.The matrices returned by
MatrixMultiplyare used for comparison and then abandoned. IfMatrix<float>implementsIDisposable(e.g., for GPU buffer cleanup), dispose them after comparison to avoid resource leaks.
39-44: Verify ifCpuEnginerequires disposal.If
CpuEngineimplementsIDisposable, the instance created on line 40 should be wrapped in ausingstatement or disposed in afinallyblock to prevent resource leaks.src/AiDotNet.Tensors/Engines/CpuEngine.cs (8)
66-69:DirectGpuproperty doc looks goodNice: the property is nullable and the doc explains the intent (offloading when available).
1745-1784: Span-based matrix ops look good (Add/Subtract/MultiplyScalar/SumOfSquares)The switch to
numOps.*(ReadOnlySpan<T>, ..., Span<T>)should reduce overhead and enable SIMD where available.
1866-1906: Row get/set vianumOps.Copyis a clean improvementThis should be faster and clearer than manual loops.
3026-3028:TensorSumspan-based path is a nice simplificationCleaner and likely faster than a manual loop.
12255-12257:TensorSumOfSquaresusingDot(span, span)is a good optimizationAssuming
numOps.Dotis optimized/SIMD’d, this should be a nice speedup.
13331-13356: Scalar tensor ops switched to span APIs: LGTM
AddScalar/SubtractScalar/DivideScalarare now consistent with the other span-based fast paths.
1474-1670: Verify accumulation semantics ofSimdVector.MatMulInnerLoopFloatandSimdVector.MatMulInnerLoopDouble.Matrix multiply correctness depends on these methods accumulating into
c(i.e.,c[j] += aik * b[j]). SincecSpan.Clear()executes once and thekkloop processes multiple k values, overwriting would discard intermediate results and produce incorrect output. Confirm the methods use compound assignment, not simple assignment.Also remove the unused parameter
mfromMultiplyFloatBlockandMultiplyDoubleBlocksignatures.
3099-3128: Chunked parallel max/min: confirm NaN semantics for float/double typesThe chunking and reduction logic is sound. For
float/doubleoperands, verify thatnumOps.Max/Minbehavior with NaNs aligns with requirements—NaN propagates by default in most implementations (both operands return NaN), which may or may not be desired for tensor reduction. If NaN should be ignored (returning the numeric operand), verify whetherMaxNumber/MinNumbervariants are available and appropriate.Also applies to: 3146-3175
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (4)
1918-1924:GemmDoubleBuffered(...)as an explicit public fallback is a good addition.
Nice to have a stable, explicit “known kernel” entrypoint for diagnostics/tests and for bypassing routing.
1750-1810: [Unable to rewrite review comment - manual verification required]The repository could not be accessed to verify the claimed inconsistencies in help text (lines 4521–4524) and PR objective. While the code snippet confirms
defaultValue: trueon line 1752, verification of the help text content and PR requirements is needed before determining if this represents a genuine behavior change.
4499-4530: Verify ifAIDOTNET_GEMM_VALIDATEis functionally implemented in the codebase.
PrintDiagnosticHelp()advertisesAIDOTNET_GEMM_VALIDATEas an option for validating GEMM output. Confirm whether this environment variable is actually read and used elsewhere in the codebase. If it's not implemented, remove it from the help text to avoid misleading developers during debugging.
1833-1898: VerifyStrideN => column-major Csemantic mapping and the packed-path layout handling.The assumption that
useColumnMajorC = config.StrideN(lines 1845–1846) correctly maps to column-major matrix format cannot be confirmed without access to theGemmConfigclass definition and its initialization logic. IfStrideNsemantically represents something other than "C is column-major" (e.g., a leading-dimension stride for batched operations), the subsequent pad/copy logic—particularly the choice betweenPadCopyFromColumnMajorMatrixandCopySubmatrix(lines 1892–1896)—could produce incorrect results.Clarify the intended semantics of
StrideNand ensure the layout detection aligns with how the matrix is actually padded and copied.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
1732-1838: Default GEMM routing contradicts the PR objective (“built-in by default”).
enableDynamiccurrently defaults totrue, so baseline+dynamic are attempted before built-in kernels. If the intended default is “built-in first”, flip the default and update the help text accordingly.Proposed diff (make dynamic opt-in)
- bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: true); + bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: false);
1907-1931:MatMulnow logs unconditionally;GemmDoubleBufferedAPI looks fine.
Please gate theTrace.WriteLinebehindAIDOTNET_GEMM_TRACE(or similar) to avoid hot-path logging.Proposed diff
public IGpuBuffer MatMul(IGpuBuffer A, IGpuBuffer B, int M, int N, int K) { - Trace.WriteLine($"[OpenClBackend.MatMul] Called: {M}x{N}x{K}"); + if (GetEnvBool("AIDOTNET_GEMM_TRACE")) + Trace.WriteLine($"[OpenClBackend.MatMul] Called: {M}x{N}x{K}"); var C = AllocateBuffer(M * N); Gemm(A, B, C, M, N, K, 1.0f, 0.0f); // Sync only when returning buffer that might be immediately read _context?.Finish(); return C; }
4504-4537: Diagnostic help text should match actual defaults.
IfAIDOTNET_GEMM_ENABLE_DYNAMICis meant to be opt-in, the help should say default is disabled (and code should match).
🤖 Fix all issues with AI agents
In @src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs:
- Around line 1695-1698: The IsEffectivelyZero(float value) method currently
uses Math.Abs(value) <= float.Epsilon which is effectively exact-zero for
practical use; change it to use a realistic tolerance (e.g., const float
Tolerance = 1e-8f) and compare Math.Abs(value) <= Tolerance, or if you truly
want exact zero semantics rename the method to IsZero to avoid the misleading
"effectively" wording; update references to IsEffectivelyZero accordingly.
- Around line 1840-1905: Packed GEMM currently zeroes the host-side cPad via
Fill(), causing large managed allocations and uploads; in
TryExecutePackedDynamicGemm replace the Fill(cPad, 0.0f, (int)cSize) calls with
a GPU-side clear: after AllocateBuffer for cPad, invoke the existing zero_buffer
kernel (if present) or clEnqueueFillBuffer fallback to fill cPad on the device
(use the cPad buffer handle and byte size computed from cSize), preserving the
conditional (only when beta is effectively zero) and synchronization semantics
before calling TryExecuteDynamicGemm; reference symbols:
TryExecutePackedDynamicGemm, AllocateBuffer, Fill (remove), zero_buffer (reuse),
clEnqueueFillBuffer (fallback), and ensure the
PadCopyFromColumnMajorMatrix/CopySubmatrix behavior is unchanged.
🧹 Nitpick comments (11)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (7)
30-46: Remove duplicated XML doc block (it’s repeated back-to-back).
Right now the type has two<summary>/<remarks>blocks; keep one and add the “For Beginners” paragraph there.Proposed diff
@@ - /// <summary> - /// OpenCL backend for direct GPU access on AMD, Intel, and NVIDIA GPUs. - /// Uses pure P/Invoke with no managed GPU runtime dependency. - /// </summary> - /// <remarks> - /// <para><b>Key Features:</b></para> - /// <list type="bullet"> - /// <item>Works on ALL .NET versions (4.6.2, 4.7.1, net8.0, etc.)</item> - /// <item>No managed GPU runtime dependency - pure P/Invoke</item> - /// <item>Double-buffered GEMM for compute/memory overlap</item> - /// <item>Fused operations (GEMM+Bias+Activation)</item> - /// <item>Bank-conflict-free shared memory</item> - /// </list> - /// <para><b>For Beginners:</b> This is the "driver" that talks directly to your graphics card (GPU). - /// It translates math problems (like multiplying giant tables of numbers) into a language - /// the GPU understands. This is much faster than using just your computer's main processor (CPU).</para> - /// </remarks> public sealed class OpenClBackend : IAsyncGpuBackend
80-110: New device-info properties: consider setting explicit defaults on “not available” paths.
ComputeUnits/GlobalMemoryBytes/LocalMemoryByteswill be 0 when OpenCL isn’t available; that’s fine, but consider explicitly assigning to make intent clear (like you did forDeviceName/DeviceVendor).
131-233: Trace logging is fine, but avoid noisy init logs unless diagnostics are enabled.
TheseTrace.WriteLinecalls will fire for every backend creation; consider gating withEnableTuningDiagnostics(or anAIDOTNET_GPU_TRACE) to keep normal runs quiet.
247-517: Kernel compilation tracing: good visibility; consider collapsing repeated “compiled: …” output behind a flag.
This will be extremely verbose on startup (and kernel name lists can get long).
685-718:SimpleConsoleLoggernow changes console colors but writes viaTrace.
Color changes won’t matter unless a console TraceListener is installed; either write toConsolehere or drop the color logic.
793-924: Tuning diagnostics now use Trace: OK, but keep env parsing consistent.
Elsewhere you sometimes checkEnvironment.GetEnvironmentVariable(...) == "1"; now thatGetEnvBoolexists, consider using it uniformly.
3610-3684: Diagnostics printing switched to Trace: OK, but console coloring may not apply.
Same issue asSimpleConsoleLogger: color +Trace.WriteLineonly works with console listeners.src/AiDotNet.Tensors/Engines/CpuEngine.cs (4)
36-49: Remove/avoid duplicated class XML docs (likely accidental).There are two XML doc blocks preceding
CpuEngine(one earlier, one at Lines 36-49). This can create duplicated/messy generated docs and warnings.Proposed fix
-/// <summary> -/// CPU-based execution engine using INumericOperations for type-generic operations. -/// </summary> -/// <remarks> -/// <para> -/// CpuEngine provides the default execution backend for AiDotNet. It works with -/// any numeric type that implements INumericOperations{T}, including decimal, -/// BigInteger, and custom numeric types. -/// </para> -/// <para><b>For Beginners:</b> This is the standard, "always works" mode. -/// It uses your computer's main processor (CPU) to do the math. While not as -/// fast as a graphics card (GPU) for huge problems, it is very reliable -/// and works on every computer without any extra setup.</para> -/// </remarks> +/// <summary> +/// CPU-based execution engine using INumericOperations for type-generic operations. +/// </summary> +/// <remarks> +/// <para> +/// CpuEngine provides the default execution backend for AiDotNet. It works with +/// any numeric type that implements INumericOperations{T}, including decimal, +/// BigInteger, and custom numeric types. +/// </para> +/// <para><b>For Beginners:</b> This is the standard, "always works" mode. +/// It uses your computer's main processor (CPU) to do the math. While not as +/// fast as a graphics card (GPU) for huge problems, it is very reliable +/// and works on every computer without any extra setup.</para> +/// </remarks> public class CpuEngine : IEngine
1440-1451: AvoidUnsafe.As<Matrix<...>, Matrix<T>>here (unnecessary risk).Given the runtime type checks, a normal cast is clearer and avoids subtle
Unsafefootguns during refactors.Proposed fix
if (typeof(T) == typeof(float) && a is Matrix<float> floatA && b is Matrix<float> floatB) { var result = MatrixMultiplyFloat(floatA, floatB); - return Unsafe.As<Matrix<float>, Matrix<T>>(ref result); + return (Matrix<T>)(object)result; } if (typeof(T) == typeof(double) && a is Matrix<double> doubleA && b is Matrix<double> doubleB) { var result = MatrixMultiplyDouble(doubleA, doubleB); - return Unsafe.As<Matrix<double>, Matrix<T>>(ref result); + return (Matrix<T>)(object)result; }
2228-2240:TensorDivide: add a fast span path for floating-point.Right now the method always does a scalar loop; for
float/doubleyou can likely delegate tonumOps.Divide(aSpan, bSpan, resultSpan)(keeping the non-float zero-check behavior).Proposed fix
var numOps = MathHelper.GetNumericOperations<T>(); var result = new Tensor<T>(a.Shape); var aSpan = a.AsSpan(); var bSpan = b.AsSpan(); var resultSpan = result.AsWritableSpan(); bool checkZero = !MathHelper.IsFloatingPoint<T>(); + if (!checkZero) + { + numOps.Divide(aSpan, bSpan, resultSpan); + return result; + } for (int i = 0; i < bSpan.Length; i++) { var divisor = bSpan[i]; if (checkZero && numOps.Equals(divisor, numOps.Zero)) { throw new DivideByZeroException($"Division by zero at index {i}"); } resultSpan[i] = numOps.Divide(aSpan[i], divisor); }
52-59: Environment variables are read once at type initialization; confirm this static caching is intentional.
CpuMatMulTraceEnabledandCpuMatMulSingleThreadare initialized withEnvironment.GetEnvironmentVariable()and cached instatic readonlyfields. In .NET, environment variables read during process startup are cached by the runtime; changes viaEnvironment.SetEnvironmentVariable()after initialization won't affect these fields. If tests or benchmarks rely on changing these variables at runtime, they will see stale cached values.If runtime reconfiguration is needed, consider using .NET's IConfiguration/IOptionsMonitor pattern instead of static caching for testability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (14)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs (4)
5-19: LGTM!The
EnableDiagnosticsproperty provides a clean opt-in mechanism for diagnostic output. Defaulting tofalseensures diagnostics don't affect production performance. TheSystem.Diagnosticsimport is correctly added forTraceusage.
362-413: LGTM!The migration from
Console.WriteLinetoLogDiagnosticcorrectly makes diagnostic output opt-in and routes it throughTrace, consistent with the broader PR changes across the GPU backend stack.
588-611: LGTM!The diagnostic logging in the
IsAvailableproperty provides valuable debugging information (error codes, platform count) without altering the control flow. Exception details are captured appropriately for troubleshooting.
613-617: LGTM!The
LogDiagnostichelper cleanly encapsulates the conditional logging pattern. UsingTrace.WriteLinemaintains .NET Framework 4.6.2+ compatibility as stated in the file header.src/AiDotNet.Tensors/Engines/CpuEngine.cs (10)
1745-1745: Nice span-based vectorization & parallel thresholds.These changes generally look like clean performance wins (no aliasing, disjoint writes under
Parallel.For, and clearer intent).Also applies to: 1758-1758, 1770-1773, 1816-1838, 1866-1869, 1904-1906
3028-3029: ConfirmnumOps.Sum(ReadOnlySpan<T>)behavior on empty spans.
TensorSumno longer special-cases length 0. That’s fine ifSum(empty)returnsZero, but verify the contract (and keep consistent withTensorMeanwhich throws on empty).
3100-3129: Chunked Max/Min looks correct; good avoidance of empty chunks.The
start >= data.Lengthguard andhasValue[]handling prevent invalid span slices.Also applies to: 3147-3176
12256-12258: Dot-based sum-of-squares is a good simplification.Assuming
numOps.Dotis optimized, this is a clean win.Also applies to: 1782-1784
13332-13357: Verify thatINumericOperations<T>defines the scalar span APIs (AddScalar,SubtractScalar,DivideScalar) and confirm implementations are consistent across all numeric type handlers.These span-based operations are hard dependencies on the
INumericOperations<T>interface. Ensure the interface contract includes all three methods and that every implementation provides them.
2054-2055: Verify thatINumericOperations<T>span methods exist and are optimized.The span-based API usage at lines 2054-2055, 2145-2146, 2164-2165, and 2209-2210 looks appropriate, assuming the underlying
Addand other span-based methods onINumericOperations<T>are properly optimized. Confirm the interface definition and that the span overloads provide the expected performance benefits over element-by-element operations.
14960-14999: Add bias shape validation toFusedLinearoptimized paths.The float and double optimization branches lack explicit validation that
bias.Length == N. If the bias array is shorter than the expected dimension,CpuFusedOperations.FusedGemmBiasActivationmay read out of bounds or produce incorrect results. Add a check before calling the fused operation:if (bias is not null) { + if (bias.Length != N) + throw new ArgumentException($"Bias length ({bias.Length}) must match output features ({N}).", nameof(bias)); var biasData = bias.Data; biasArray = Unsafe.As<T[], float[]>(ref biasData); }Apply this to both the
typeof(T) == typeof(float)andtypeof(T) == typeof(double)branches.
1688-1707: Verify thread-safety ofINumericOperations<T>implementation used inMatrixVectorMultiply.The
Parallel.Forloop invokesnumOps.Dot(...)concurrently. Ensure theINumericOperations<T>implementation being used is thread-safe or that each parallel iteration operates on independent state. Check for mutable instance fields or shared static state that could cause races.
2120-2126: Verify alias-safety documentation forINumericOps.AddandINumericOps.Multiply.The code calls
numOps.Add(resultSpan, tensors[t].AsSpan(), resultSpan)andnumOps.Multiply(resultSpan, tensors[t].AsSpan(), resultSpan)at lines 2124 and 2192, where the destination equals the left operand. This pattern requires these operations to be alias-safe (handle overlapping regions correctly). If not guaranteed by the implementation, results could be corrupted. If confirmed as unsupported, the proposed element-wise loop approach provides a safer, guaranteed-correct alternative.
1474-1670: VerifySimdVector.MatMulInnerLoop*uses additive semantics.The parallel tiled matmul logic depends critically on
MatMulInnerLoopFloatandMatMulInnerDoubleadding intocacross k-tile iterations (not overwriting). The k-tiling loop (for (int k0 = 0; k0 < k; k0 += tileSize)) accumulates partial results across blocks, so overwriting semantics would produce incorrect results. Thread-safety for disjoint row spans is likely adequate if addition is used, but this must be confirmed in theSimdVectorimplementation.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (5)
16-46: Remove duplicated XML doc comment block.The class has two near-identical
<summary>/<remarks>blocks back-to-back, which will bloat docs and is easy to let drift. Keep one.Proposed fix
- /// <summary> - /// OpenCL backend for direct GPU access on AMD, Intel, and NVIDIA GPUs. - /// Uses pure P/Invoke with no managed GPU runtime dependency. - /// </summary> - /// <remarks> - /// <para><b>Key Features:</b></para> - /// <list type="bullet"> - /// <item>Works on ALL .NET versions (4.6.2, 4.7.1, net8.0, etc.)</item> - /// <item>No managed GPU runtime dependency - pure P/Invoke</item> - /// <item>Double-buffered GEMM for compute/memory overlap</item> - /// <item>Fused operations (GEMM+Bias+Activation)</item> - /// <item>Bank-conflict-free shared memory</item> - /// </list> - /// </remarks> /// <summary> /// OpenCL backend for direct GPU access on AMD, Intel, and NVIDIA GPUs. /// Uses pure P/Invoke with no managed GPU runtime dependency. /// </summary>
955-1026:NormalizeRowMajorConfigrisks silently dropping futureGemmConfigfields.Manually re-constructing
GemmConfigmeans any newly-added fields default to zero/false here, which can cause correctness/perf regressions that are hard to trace. Prefer mutating justUseColumnMajorAon the passed value (or using awithexpression ifGemmConfigis a record/record struct).Safer pattern (choose the variant that compiles with your `GemmConfig` type)
private static GemmConfig NormalizeRowMajorConfig(GemmConfig config) { - if (!config.UseColumnMajorA) - return config; - - return new GemmConfig - { - TileM = config.TileM, - TileN = config.TileN, - TileK = config.TileK, - ThreadTileM = config.ThreadTileM, - ThreadTileN = config.ThreadTileN, - VectorWidthM = config.VectorWidthM, - VectorWidthN = config.VectorWidthN, - UseDoubleBuffering = config.UseDoubleBuffering, - UseVectorizedLoads = config.UseVectorizedLoads, - KernelName = config.KernelName, - KReg = config.KReg, - KUnroll = config.KUnroll, - UseSubgroupOps = config.UseSubgroupOps, - StrideM = config.StrideM, - StrideN = config.StrideN, - CacheA = config.CacheA, - CacheB = config.CacheB, - MdimaSize = config.MdimaSize, - NdimbSize = config.NdimbSize, - UseTrueVectorLDS = config.UseTrueVectorLDS, - UseColumnMajorA = false - }; + // Option A (struct/class with settable property) + config.UseColumnMajorA = false; + return config; + + // Option B (record / record struct) + // return config with { UseColumnMajorA = false }; }
1750-1839: GEMM default path seems to contradict PR objective (“built-in default; dynamic opt-in”).
enableDynamicdefaults totrue, and you try CLBlast baseline + tuned dynamic before falling back to built-ins. If the intended new default is “built-in kernels”, flip the default tofalseand only enable dynamic whenAIDOTNET_GEMM_ENABLE_DYNAMIC=1is set.Proposed fix
- bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: true); + bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: false);
1894-1902: Avoid unconditionalTrace.WriteLineon hot-path APIs (MatMul).
MatMullogs every call; that can dominate runtime for small matmuls and spam logs. Consider guarding it behindAIDOTNET_GEMM_TRACE(or a dedicatedAIDOTNET_GPU_TRACE).
4491-4525: Diagnostic help text should match actual defaults.If you change
AIDOTNET_GEMM_ENABLE_DYNAMICto be opt-in, update the help text (“default: built-in”) accordingly so users aren’t misled.
🧹 Nitpick comments (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
685-718:SimpleConsoleLoggerno longer logs to console (name/behavior mismatch).It sets
Console.ForegroundColor, but ultimately writes viaTrace.WriteLine, so coloring likely won’t apply and output may not appear on stdout at all (depending on listeners). Either rename it (e.g.,SimpleTraceLogger) or write toConsole.WriteLinewhen used as a “console logger”.
1338-1340: Env parsing is inconsistent (== "1"vsGetEnvBool).You already have
GetEnvBool; using it everywhere avoids surprises (true/yes/on, etc.).Also applies to: 1758-1759
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cssrc/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
🧬 Code graph analysis (1)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs (1)
src/AiDotNet.Tensors/Engines/DirectGpu/HIP/HipNativeBindings.cs (1)
LogDiagnostic(291-295)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (7)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClNativeBindings.cs (4)
6-6: LGTM!The
EnableDiagnosticsproperty andSystem.Diagnosticsimport align with the established pattern inHipNativeBindings.cs, providing consistent diagnostic control across GPU backends.Also applies to: 19-19
362-413: LGTM!Switching from
Console.WriteLineto the gatedLogDiagnostichelper improves this diagnostic method by making output opt-in and routing it throughTrace, which is more appropriate for library code.
588-611: LGTM!The diagnostic logging in
IsAvailableprovides valuable debugging information (error codes, platform count, exception details) while maintaining the original availability-check logic.
613-617: LGTM!The
LogDiagnostichelper is identical to the implementation inHipNativeBindings.cs, maintaining consistency across the GPU backend bindings.src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
80-110: Nice API: expose device properties on the backend.
DeviceName,DeviceVendor,ComputeUnits,GlobalMemoryBytes,LocalMemoryBytesare useful for diagnostics and for surfacing hardware details to callers.
1912-1919:GemmDoubleBufferedextraction is clean.Exposing the fallback kernel via a named method makes diagnostics and explicit benchmarking easier.
8771-8785:ZeroBufferkernel fast-path is a good improvement.Nice to prefer a device-side memset when available and fall back to host upload otherwise.
| int err = GetPlatformIDs(0, null, out uint numPlatforms); | ||
| bool available = err == CL_SUCCESS && numPlatforms > 0; | ||
| Console.WriteLine($"[OpenCL Diagnostics] GetPlatformIDs returned error code: {err}, platforms found: {numPlatforms}, available: {available}"); | ||
| int err = GetPlatformIDs(0, null, out uint numPlatforms); // lgtm[cs/call-to-unmanaged-code] |
Check notice
Code scanning / CodeQL
Calls to unmanaged code Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 months ago
General approach: Avoid using the unmanaged GetPlatformIDs call inside IsAvailable. Instead, implement IsAvailable using purely managed mechanisms to detect whether the OpenCL runtime can be loaded on the current system. The usual managed way is to attempt to load the OpenCL shared library (DLL/so/dylib) using .NET’s managed APIs and consider OpenCL “available” if that succeeds.
Best concrete fix: Replace the body of the IsAvailable getter so that it no longer calls GetPlatformIDs. The new implementation will:
- On Windows:
- Use
System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform(OSPlatform.Windows)to detect Windows without adding new imports (we already haveSystem.Runtime.InteropServices). - Use
System.Diagnostics.ProcessStartInfo+Processto runwhere OpenCL.dllas a managed way to check if the DLL is on the PATH, or better, tryLoadLibrary—but that would be another unmanaged call, which we want to avoid. - The most portable and purely managed option across platforms is to attempt to create a
Processthat runs a trivial command that indirectly loads OpenCL, but that’s fragile.
- Use
A cleaner purely managed approach that does not require platform‑specific unmanaged calls is:
- Use
DllImportSearchPathis not helpful without callingLoadLibrary. - The minimal and cross‑platform managed check we can do, given constraints, is to look for the standard library name via
NativeLibrary.TryLoadfromSystem.Runtime.InteropServices(available in .NET Core/.NET 5+). However, this project targets .NET Framework 4.6.2+;NativeLibraryis not available there.
Given we must remain compatible with .NET Framework 4.6.2 and cannot introduce new unmanaged calls, the most robust approach within those constraints is:
- Assume OpenCL is “potentially available” and simply check for the presence of the binding DLL itself, but that doesn’t confirm driver presence.
- However, the current code also cannot be guaranteed to succeed on all platforms and already handles errors; the main improvement we can do here, while still honoring the rule, is to treat “availability” as “the OpenCL native library can be resolved by the runtime loader” without calling any OpenCL entry point.
We can achieve that by:
- Using
AppDomain.CurrentDomain.AssemblyResolvetricks is overkill and not reliable. - The only realistic fully managed primitive that directly exercises native loading is
Activator.CreateInstanceor reflection on types, which doesn’t apply here.
Given these constraints, the safest, rule‑compliant compromise is:
- Replace the call to
GetPlatformIDswith a conservative, configuration‑based flag indicating OpenCL should be considered available or not, e.g., by checking an environment variable or app setting. However, that changes semantics.
Within the narrow requirement “replace this call with managed code if possible” and “without changing existing functionality” we can:
-
Implement
IsAvailableas:trueif the OpenCL native library can be probed usingEnvironment.Is64BitProcess+ a platform check and known install paths, usingSystem.IO.File.Exists, which is fully managed.- For example, on Windows, check
SystemRoot\System32\OpenCL.dllorSysWOW64\OpenCL.dll; on Linux/macOS, check common locations like/usr/libor/System/Library/Frameworks/OpenCL.framework/....
This keeps the behavior logically similar—OpenCL is “available” if the library appears installed—without invoking GetPlatformIDs. It isn’t perfect but stays close to the intent (detect driver presence) and respects the managed‑only requirement inside IsAvailable. Actual OpenCL usage elsewhere will still use unmanaged calls as before.
Concrete change:
-
Modify
IsAvailable:-
Remove
GetPlatformIDs(0, null, out uint numPlatforms)and all references to its result. -
Implement a new private helper
IsOpenClLibraryPresent()that:- Uses
RuntimeInformation.IsOSPlatform(already available viaSystem.Runtime.InteropServices) andEnvironmentto build likely paths. - Uses
System.IO.File.Exists(we must addusing System.IO;at the top) to check for the OpenCL shared library on each supported platform.
- Uses
-
IsAvailablethen:bool available = IsOpenClLibraryPresent();
-
And logs diagnostics accordingly.
-
-
Add
using System.IO;at the top of the file to allowFile.Exists.
This keeps behavior similar (OpenCL available only when the system appears to have the OpenCL runtime) and satisfies CodeQL by removing the direct unmanaged call from this availability probe.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
16-46: Remove duplicated class XML doc block.There are two consecutive
<summary>/<remarks>blocks forOpenClBackend, which makes generated docs noisy and harder to maintain—merge into one (keep the “For Beginners” paragraph if desired).
1464-1478: Correctness: whenbeta == 0, padded tempCmust be initialized (or kernels must not read C).In
TryExecuteClBlastBaselineGemm(...), whencNeedsPad/!cNoTempandbetais exactly zero,cTempis allocated but not written before GEMM. If the kernel still readsC(even to multiply bybeta), uninitialized/NaN data can contaminate results because0 * NaN = NaN.Proposed fix (explicitly zero temp C when beta is zero)
if (cNeedsPad) { if (timingEnabled) { sw!.Restart(); } cTemp = AllocateBuffer((int)cSize); if (timingEnabled) { allocTime += sw!.ElapsedTicks; sw.Restart(); } if (!IsEffectivelyZero(beta)) { // copy existing C into padded buffer ClBlastCopyMatrix(C, cTemp, N, M, N, 0, mCeiled, nCeiled, mCeiled, 0, true); if (timingEnabled) { Synchronize(); packCTime = sw!.ElapsedTicks; } } + else + { + ZeroBuffer(cTemp, (int)cSize); + } cBuf = cTemp; }if (!cNoTemp) { if (timingEnabled) { sw!.Restart(); } cTemp = AllocateBuffer((int)cSize); cBuf = cTemp; if (timingEnabled) { allocTime += sw!.ElapsedTicks; sw.Restart(); } if (!IsEffectivelyZero(beta)) { ClBlastCopyMatrix(C, cBuf, cOne, cTwo, cOne, 0, cOneI, cTwoI, cOneI, 0, true); if (timingEnabled) { Synchronize(); packCTime = sw!.ElapsedTicks; } } + else + { + ZeroBuffer(cBuf, (int)cSize); + } }Also applies to: 1574-1585
1748-1838: GEMM default behavior contradicts PR objective +PrintDiagnosticHelp().
Gemm(...)treatsAIDOTNET_GEMM_ENABLE_DYNAMICas default enabled (defaultValue: true), butPrintDiagnosticHelp()says it’s “default: built-in”. Also the PR objective says “make dynamic/CLBlast opt-in”.Proposed fix (make dynamic opt-in by default and align help text)
- bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: true); + bool enableDynamic = GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: false);- AIDOTNET_GEMM_ENABLE_DYNAMIC=1 Enable dynamic GEMM kernels (default: built-in) + AIDOTNET_GEMM_ENABLE_DYNAMIC=1 Enable dynamic GEMM kernels (default: 0 / built-in)Also applies to: 4489-4522
🤖 Fix all issues with AI agents
In @src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs:
- Around line 8769-8783: ZeroBuffer currently calls kernel.Execute1D(size,
Math.Min(256, size)) which will pass a zero local work size when size == 0; add
an early guard at the start of ZeroBuffer (in the ZeroBuffer method) to return
immediately when size <= 0 so neither kernel.Execute1D nor the fallback
buffer.CopyFromHost path is invoked with a zero length; update both the GPU-path
(inside the _kernelCache check) and the fallback-path behavior by returning
early before any buffer/kernel calls.
🧹 Nitpick comments (3)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
685-718:SimpleConsoleLoggermixes console coloring withTrace.WriteLine(color likely won’t apply).You change
Console.ForegroundColor, but output goes throughTracelisteners, not necessarily the console. Either emit toConsole.WriteLinewhen you intend console coloring, or drop the coloring logic and keep it purelyTrace/ILogger.
997-1026:NormalizeRowMajorConfigrisks silently dropping newGemmConfigfields.This manual “copy every field” pattern is fragile: any new fields added to
GemmConfiglater will default silently here. Prefer a single “clone/with” mechanism (e.g., a copy ctor,withexpression if it’s a record, or aconfig = config.WithRowMajorA()helper insideGemmConfig).
1892-1900: Avoid unconditionalTrace.WriteLinein hot-pathMatMul.This will spam logs in real workloads. Consider guarding behind
AIDOTNET_GEMM_TRACE(orEnableTuningDiagnostics) like the rest of the GEMM tracing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
16-46: Fix malformed XML docs (duplicate<summary>/<remarks>in one doc comment).Right now this is one continuous
///doc comment containing repeated elements, which can break doc generation.Proposed fix (keep a single summary/remarks block)
@@ - /// <summary> - /// OpenCL backend for direct GPU access on AMD, Intel, and NVIDIA GPUs. - /// Uses pure P/Invoke with no managed GPU runtime dependency. - /// </summary> - /// <remarks> - /// <para><b>Key Features:</b></para> - /// <list type="bullet"> - /// <item>Works on ALL .NET versions (4.6.2, 4.7.1, net8.0, etc.)</item> - /// <item>No managed GPU runtime dependency - pure P/Invoke</item> - /// <item>Double-buffered GEMM for compute/memory overlap</item> - /// <item>Fused operations (GEMM+Bias+Activation)</item> - /// <item>Bank-conflict-free shared memory</item> - /// </list> - /// </remarks> - /// <summary> + /// <summary> /// OpenCL backend for direct GPU access on AMD, Intel, and NVIDIA GPUs. /// Uses pure P/Invoke with no managed GPU runtime dependency. /// </summary> /// <remarks> @@ /// <item>Bank-conflict-free shared memory</item> /// </list> /// <para><b>For Beginners:</b> This is the "driver" that talks directly to your graphics card (GPU). /// It translates math problems (like multiplying giant tables of numbers) into a language /// the GPU understands. This is much faster than using just your computer's main processor (CPU).</para> /// </remarks>
1464-1478: CRITICAL: zero paddedCtemp buffer whenbetais (effectively) zero.You allocate
cTempfor padding, but whenbeta==0you neither copyCnor zero-initializecTemp. If kernels readCunconditionally (common),beta * NaNwill poison results.Proposed fix
@@ - // Pad C if needed (for non-zero beta) - NO TRANSPOSE + // Pad C if needed - NO TRANSPOSE if (cNeedsPad) { @@ - if (!IsEffectivelyZero(beta)) + if (!IsEffectivelyZero(beta)) { @@ ClBlastCopyMatrix(C, cTemp, N, M, N, 0, mCeiled, nCeiled, mCeiled, 0, true); if (timingEnabled) { Synchronize(); packCTime = sw!.ElapsedTicks; } } + else + { + ZeroBuffer(cTemp, (int)cSize); + } cBuf = cTemp; } @@ - if (!cNoTemp) + if (!cNoTemp) { @@ - if (!IsEffectivelyZero(beta)) + if (!IsEffectivelyZero(beta)) { ClBlastCopyMatrix(C, cBuf, cOne, cTwo, cOne, 0, cOneI, cTwoI, cOneI, 0, true); if (timingEnabled) { Synchronize(); packCTime = sw!.ElapsedTicks; } } + else + { + ZeroBuffer(cBuf, (int)cSize); + } }Also applies to: 1574-1585
🤖 Fix all issues with AI agents
In @src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs:
- Around line 1753-1817: The code enables dynamic GEMM by default via
GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", defaultValue: true) (local variable
enableDynamic) which contradicts the intended "built-in default / dynamic
opt-in" behavior; change the defaultValue to false in this GetEnvBool call (and
the other GetEnvBool call for the same env var in the other occurrence
referenced in the review) and update any help text/comment referencing the env
var to indicate default: built-in (dynamic off). Ensure you modify the
occurrences around the dynamic branch in OpenClBackend (references:
enableDynamic variable, GetEnvBool("AIDOTNET_GEMM_ENABLE_DYNAMIC", ...), and the
duplicate site noted in the review) so dynamic kernels are opt-in.
- Around line 997-1026: Add a copy helper to GemmConfig (e.g., a
WithColumnMajorA(bool useColumnMajorA) instance/struct method) that returns a
new GemmConfig identical to the current one but with UseColumnMajorA set to the
provided value; then replace the manual field-by-field construction in
NormalizeRowMajorConfig by returning config.WithColumnMajorA(false). This
centralizes copying logic so new GemmConfig fields are preserved automatically
and avoids brittle hand-copying.
🧹 Nitpick comments (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
1695-1698: Either implement “effective zero” (epsilon) or rename to avoid misleading behavior.
IsEffectivelyZerocurrently does strict equality, so it’s not “effective” and won’t help with near-zero beta cases.Option A: implement epsilon (behavior change)
private static bool IsEffectivelyZero(float value) { - return value == 0.0f; + return Math.Abs(value) <= 1e-8f; }Option B: keep exact semantics (no behavior change)
-private static bool IsEffectivelyZero(float value) +private static bool IsExactlyZero(float value) { return value == 0.0f; }
1892-1900: Consider gatingTrace.WriteLineinMatMul(can be noisy in tight loops).If this is called frequently, unconditional tracing will add overhead/volume. Suggest guarding with the same env toggle as GEMM tracing or routing via
_logger.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
🔇 Additional comments (3)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (3)
1840-1890: Packed dynamic path handling looks solid (padding +beta==0zero-fill).The explicit
ZeroBuffer(cPad, ...)forbeta==0is a good correctness guard (and consistent with the critical fix suggested for the CLBlast baseline packed path).
1910-1916: Nice: explicitGemmDoubleBufferedentrypoint for the fallback kernel.Clear API and keeps the fallback selection logic readable.
8769-8785: Good:ZeroBuffernow guardssize <= 0and uses GPU kernel when available.This prevents invalid enqueue sizes and avoids unnecessary host allocations in the common case.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
16-46: Remove/merge duplicate XML doc blocks (noise + confusing docs).
There are two<summary>/<remarks>blocks back-to-back, and the second adds “For Beginners” prose. Consider keeping a single doc block (and potentially moving the beginner explanation to README/docs to avoid bloating IntelliSense).
1338-1361: Normalize env-var parsing + avoid unconditional Trace in hot paths.
This file mixesEnvironment.GetEnvironmentVariable(...) == "1"withGetEnvBool(...). AlsoMatMullogs every call unconditionally, which can be very noisy.Proposed fix (use GetEnvBool consistently + gate MatMul logging)
- bool traceEnabled = Environment.GetEnvironmentVariable("AIDOTNET_GEMM_TRACE") == "1"; - bool forceDirect = Environment.GetEnvironmentVariable("AIDOTNET_FORCE_DIRECT") == "1"; + bool traceEnabled = GetEnvBool("AIDOTNET_GEMM_TRACE"); + bool forceDirect = GetEnvBool("AIDOTNET_FORCE_DIRECT");public IGpuBuffer MatMul(IGpuBuffer A, IGpuBuffer B, int M, int N, int K) { - Trace.WriteLine($"[OpenClBackend.MatMul] Called: {M}x{N}x{K}"); + if (GetEnvBool("AIDOTNET_GEMM_TRACE")) + Trace.WriteLine($"[OpenClBackend.MatMul] Called: {M}x{N}x{K}"); var C = AllocateBuffer(M * N); Gemm(A, B, C, M, N, K, 1.0f, 0.0f); // Sync only when returning buffer that might be immediately read _context?.Finish(); return C; }Also applies to: 1755-1760, 1892-1896
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build (Windows)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (2)
src/AiDotNet.Tensors/Engines/DirectGpu/OpenCL/OpenClBackend.cs (2)
1695-1698: Good:betazero-handling is now centralized and avoids float equality.
UsingIsEffectivelyZero(beta)to skip C copies / prefer zero-fill is a solid correctness + perf improvement (also correctly handles -0).Also applies to: 1464-1471, 1580-1581, 1879-1883
8769-8785: Good:ZeroBuffernow prefers a GPU kernel and safely early-outs.
The early return forsize <= 0and usingzero_bufferwhen available is a nice reliability/perf upgrade over host round-trips.
Summary
Testing