Remove bounds checks for Log2 function in FormattingHelpers.CountDigits by EgorBo · Pull Request #113790 · dotnet/runtime

EgorBo · 2025-03-22T14:58:36Z

And replaces unsafe array access in FormattingHelpers.CountDigits.cs

Example:

nint CountDigits(ulong value)
{
    ReadOnlySpan<byte> log2ToPow10 =
    [
        1,  1,  1,  2,  2,  2,  3,  3,  3,  4,  4,  4,  4,  5,  5,  5,
        6,  6,  6,  7,  7,  7,  7,  8,  8,  8,  9,  9,  9,  10, 10, 10,
        10, 11, 11, 11, 12, 12, 12, 13, 13, 13, 13, 14, 14, 14, 15, 15,
        15, 16, 16, 16, 16, 17, 17, 17, 18, 18, 18, 19, 19, 19, 19, 20
    ];
    return log2ToPow10[(int)ulong.Log2(value)];
}

Was:

; Assembly listing for method C:CountDigits(ulong):long:this (FullOpts)
       sub      rsp, 40
       or       rdx, 1
       xor      eax, eax
       lzcnt    rax, rdx
       xor      eax, 63
       cmp      eax, 64
       jae      SHORT G_M15126_IG04
       mov      rcx, 0x2058DCA2A08
       movzx    rax, byte  ptr [rax+rcx]
       add      rsp, 40
       ret      
G_M15126_IG04:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code 48

Now:

; Assembly listing for method C:CountDigits(ulong):long (FullOpts)
       or       rcx, 1
       xor      eax, eax
       lzcnt    rax, rcx
       xor      eax, 63
       mov      rcx, 0x2452FB82A08
       movzx    rax, byte  ptr [rax+rcx]
       ret      
; Total bytes of code 29

Copilot

Pull Request Overview

This PR removes bounds checks and unsafe array access in the Log2-based digit lookup in FormattingHelpers.CountDigits, improving both performance and clarity.

Replaces Unsafe.Add pointer arithmetic with direct array indexing for the ulong-based digit lookup.
Applies the same change for the uint-based computation using a lookup table.

Files not reviewed (3)

src/coreclr/jit/rangecheck.cpp: Language not supported
src/coreclr/jit/valuenum.cpp: Language not supported
src/coreclr/jit/valuenum.h: Language not supported

EgorBo · 2025-03-24T07:37:44Z

@jakobbotsch @dotnet/jit-contrib PTAL - removes unsafe array access from two places. Just a couple of diffs, but, hopefully, this introduces more convenient helpers to do pattern matching on top of VNs. the GetVNFunc patter is quite verbose

EgorBo · 2025-03-24T07:39:15Z

src/coreclr/jit/rangecheck.cpp

 Range RangeCheck::ComputeRangeForBinOp(BasicBlock* block, GenTreeOp* binop, bool monIncreasing DEBUGARG(int indent))
 {
-    assert(binop->OperIs(GT_ADD, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL));
+    assert(binop->OperIs(GT_ADD, GT_XOR, GT_AND, GT_RSH, GT_RSZ, GT_LSH, GT_UMOD, GT_MUL));


NOTE: ideally, we should have handled XOR/OR/HWINTRINSIC all separately for GetRange, but it seems that it has no diffs outside of Log2 pattern, so I decided to just do an ad-hoc Log2 matching.

tannergooding · 2025-03-24T16:05:22Z

src/coreclr/jit/valuenum.cpp

+    int      xorBy;
+    ValueNum op;
+    // First, see if it's "X ^ 31" or "X ^ 63".
+    if (IsVNBinFuncWithConst(vn, VNF_XOR, &op, &xorBy) && ((xorBy == 31) || (xorBy == 63)))


nit: This is missing handling for X86Base_BitScanReverse which is used for R2R and NAOT, since LZCNT isn't baseline.

tannergooding · 2025-03-24T16:07:23Z

src/coreclr/jit/valuenum.cpp

+#endif
+        // Next, see if it's "LZCNT32(X | 1)" or "LZCNT64(X | 1)".
+        int orBy;
+        if (IsVNBinFunc(op, lzcntFunc, &op) && IsVNBinFuncWithConst(op, VNF_OR, &op, &orBy) && (orBy == 1))


Just noting, that X | 1 isn't required if we know that X != 0, but its probably not necessary to handle.

xtqqczze · 2025-03-25T17:05:57Z

@MihuBot

jakobbotsch · 2025-03-25T17:12:28Z

@xtqqczze for changes that do not change JIT-EE GUID and with few misses we generally use superpmi-diffs, which has much wider and generally more realistic coverage than PMI based diffs. You can see that @EgorBo linked to those above (press the "Extensions" tab).

jakobbotsch · 2025-03-25T17:15:33Z

src/coreclr/jit/valuenum.h

+    bool IsVNBinFunc(ValueNum vn, VNFunc func, ValueNum* op1 = nullptr, ValueNum* op2 = nullptr);
+
+    // Returns "true" iff "vn" is a function application of the form "func(op, cns)"
+    // the cns can be on the left side if the function is commutative.
+    template <typename T>
+    bool IsVNBinFuncWithConst(ValueNum vn, VNFunc func, ValueNum* op, T* cns)


Nit: Other functions in this class seem to write out Binary in full (e.g. EvalMathFuncBinary above).

Feel free to ignore or address as part of a separate PR.

stephentoub · 2025-05-29T02:43:27Z

@EgorBo, can this be merged?

EgorBo · 2025-05-29T17:29:52Z

@EgorBo, can this be merged?

Yeah, I wanted to generalize the jit part a bit, but I think I'll merge as is, let me check again

EgorBo · 2025-05-30T15:09:35Z

/ba-g same issue as #116041

Remove bounds checks for Log2 function

f7e997a

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 22, 2025

EgorBo added the reduce-unsafe label Mar 22, 2025

dotnet-policy-service bot assigned EgorBo Mar 22, 2025

EgorBo added 6 commits March 22, 2025 16:30

clean up

6132b52

Fix compilation and arm

44c430a

clean up

cdc449d

Cleanup

4f61374

move to vn

73d0029

Remove redundant constant swap logic

7291f0c

build-analysis bot mentioned this pull request Mar 23, 2025

Segmentation fault running System.Security.Cryptography.Tests #113785

Closed

EgorBo marked this pull request as ready for review March 24, 2025 07:35

Copilot AI review requested due to automatic review settings March 24, 2025 07:35

Copilot AI reviewed Mar 24, 2025

View reviewed changes

EgorBo requested a review from jakobbotsch March 24, 2025 07:37

EgorBo commented Mar 24, 2025

View reviewed changes

tannergooding reviewed Mar 24, 2025

View reviewed changes

tannergooding approved these changes Mar 24, 2025

View reviewed changes

MihuBot mentioned this pull request Mar 25, 2025

[JitDiff X64] [EgorBo] Remove bounds checks for Log2 function in FormattingH ... MihuBot/runtime-utils#1053

Open

jakobbotsch reviewed Mar 25, 2025

View reviewed changes

jakobbotsch approved these changes Mar 25, 2025

View reviewed changes

stephentoub approved these changes May 29, 2025

View reviewed changes

Merge branch 'main' into fix-log2-bc

cc95b4e

build-analysis bot mentioned this pull request May 29, 2025

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

This was referenced May 29, 2025

Test failure: baseservices/exceptions/stackoverflow/stackoverflowtester/stackoverflowtester.cmd #110173

Open

System.OperationCanceledException : The operation was canceled. dotnet/dnceng#5278

Closed

EgorBo merged commit a950953 into dotnet:main May 30, 2025
155 of 157 checks passed

EgorBo deleted the fix-log2-bc branch May 30, 2025 15:10

LoopedBard3 mentioned this pull request Jun 3, 2025

[Perf] Windows/x64: 8 Regressions on 5/30/2025 5:28:22 PM +00:00 #116273

Closed

github-actions bot locked and limited conversation to collaborators Jun 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove bounds checks for Log2 function in FormattingHelpers.CountDigits#113790

Remove bounds checks for Log2 function in FormattingHelpers.CountDigits#113790
EgorBo merged 8 commits intodotnet:mainfrom
EgorBo:fix-log2-bc

EgorBo commented Mar 22, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

EgorBo commented Mar 24, 2025

Uh oh!

EgorBo Mar 24, 2025

Uh oh!

tannergooding Mar 24, 2025

Uh oh!

tannergooding Mar 24, 2025

Uh oh!

xtqqczze commented Mar 25, 2025

Uh oh!

jakobbotsch commented Mar 25, 2025 •

edited

Loading

Uh oh!

jakobbotsch Mar 25, 2025

Uh oh!

stephentoub commented May 29, 2025

Uh oh!

EgorBo commented May 29, 2025

Uh oh!

EgorBo commented May 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

EgorBo commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

EgorBo commented Mar 24, 2025

Uh oh!

EgorBo Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

tannergooding Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

tannergooding Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

xtqqczze commented Mar 25, 2025

Uh oh!

jakobbotsch commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

stephentoub commented May 29, 2025

Uh oh!

EgorBo commented May 29, 2025

Uh oh!

EgorBo commented May 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EgorBo commented Mar 22, 2025 •

edited

Loading

jakobbotsch commented Mar 25, 2025 •

edited

Loading