Uh oh!
There was an error while loading. Please reload this page.
Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs - #75857
Conversation
ghost
commented
Sep 19, 2022
Tagging subscribers to this area: @dotnet/area-system-memory Issue DetailsThis resolves #75792
|
adamsitnik
left a comment
There was a problem hiding this comment.
@tannergooding big thanks for your help! PTAL at my comments, I think I've found a bug
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
tannergooding
commented
Sep 20, 2022
@stephentoub, would appreciate a secondary sign-off from you before merging. Great to have a second pair of eyes since we're backporting for .NET 7 |
| } | ||
| return ComputeLastIndex(ref searchSpace, ref currentSearchSpace, equals); | ||
| return ComputeLastIndex(offset, equals); |
There was a problem hiding this comment.
It doesn't affect this PR, but I wonder why it's written like this rather than:
if(equals!=Vector128<TValue>.Zero){returnComputeLastIndex(offset,equals);}Is this just style, or we expect the not found case to be the most common and want the branch predictor to default to that?
There was a problem hiding this comment.
we expect the not found case to be the most common and want the branch predictor to default to that
That was my assumption when I was implementing it.
tannergooding
commented
Sep 20, 2022
/backport to release/7.0-rc2 |
Started backporting to release/7.0-rc2: https://github.com/dotnet/runtime/actions/runs/3089699377 |
) The general SIMD and hardware-intrinsics guidance now lives in the official docs, which were recently rewritten in dotnet/docs#54834. Our in-repo `docs/coding-guidelines/vectorization-guidelines.md` had become largely duplicative and out of sync, so this trims it down to defer to the official article and keep only the dotnet/runtime-specific nuance: - The `BoundedMemory` test helper for access-violation testing (with a repo-relative link and a short usage example). - The real `LastIndexOf` GC-hole case study (#73768 / fix#75857) behind the "prefer the element-offset overloads" rule. - The #64451 catalogue of real vectorized implementations to learn from. The lane-crossing worked example that the official rewrite dropped is being restored upstream in dotnet/docs#54840 rather than kept here, since it''s general teaching content and belongs where it stays in sync. ---------- Also adds a `vectorization` skill covering both authoring and reviewing SIMD code. Vectorization is a cross-cutting domain rather than a folder, so a task-triggered skill fits better than a file-scoped `.instructions.md`. The skill defers to the official docs and carries the same repo-specific nuance (prefer the span/`LoadUnsafe` element-offset overloads, `BoundedMemory` AV testing, the acceleration-toggle env vars, `char`/`bool` reinterpretation, and benchmarking before committing complexity), plus authoring/testing/review checklists. `copilot-instructions.md` gets a one-line nudge toward it, matching the existing `code-review`/`performance-benchmark` pattern. > [!NOTE] > This PR description was drafted by GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This resolves#75792