Unsafe.Unlikely (Assume) draft implementation

[EgorBo]

Just a quick draft for #4966 for both RyuJIT CoreCLR and Mono-LLVM (maybe it will help to push it forward since it's 4 years old).

It allows programmers to give hints to JIT about branches' probabilities, e.g. to avoid goto hacks like this in perf critical code.

Sample:

static int Foo(int a)
{
    if (Unsafe.Unlikely(a <= 0))   // hint: `a` is unlikely to be <= 0
        return a * a; 

    return 0;
}

Codegen without Unlikely:

       85D2                 test     edx, edx
       7F06                 jg       SHORT G_M36845_IG05
       8BC2                 mov      eax, edx
       0FAFC2               imul     eax, edx
       C3                   ret                    ;; return a * a
G_M36845_IG05:
       8BC2                 mov      eax, edx
       C3                   ret                    ;; return 0

Codgen with Unlikely (this is what this PR emits)

       85D2                 test     edx, edx
       7E03                 jle      SHORT G_M4270_IG05
       8BC2                 mov      eax, edx
       C3                   ret                     ;; return 0
G_M4270_IG05:
       8BC2                 mov      eax, edx
       0FAFC2               imul     eax, edx 
       C3                   ret                     ;; return a * a 

Also, I implemented it for Mono-LLVM (I use @llvm.expect.i32 intrinsic, which is emitted when you use __builtin_expect in C/C++)

Mono-LLVM without Unlikely (macOS)

	mov	ecx, edi
	imul	ecx, ecx   ;; Mono-LLVM emits branchless code for our sample
	xor	eax, eax   
	test	edi, edi
	cmovle	eax, ecx   
	ret

Mono-LLVM with Unlikely (macOS)

	xor	eax, eax
	test	edi, edi
	jle	1 <gram_Foo1__int_+0x7>   ;; branch is better than cmove in this case
	ret                               ;; return 0
	imul	edi, edi  
	mov	eax, edi
	ret                               ;; return a * a

cc @jkotas

[vargaz]

The mono parts look ok to me.
Shouldn't there be a Likely() as well ?

[EgorBo]

The mono parts look ok to me.
Shouldn't there be a Likely() as well ?

I guess it's more an API design question, e.g. in the original issue it's proposed to be Assume(x, y). So it needs API review first.

[stephentoub]

Just a quick draft for #4966 for both RyuJIT CoreCLR and Mono-LLVM

Thanks. Is there any reason this need to be a PR? Can you not just link to the relevant commit from that issue? This doesn't seem actionable.

[EgorBo]

Just a quick draft for #4966 for both RyuJIT CoreCLR and Mono-LLVM

Thanks. Is there any reason this need to be a PR? Can you not just link to the relevant commit from that issue? This doesn't seem actionable.

Ok, will close it if there won't be any interest in some short term. Next time will use commits instead if you don't mind.

[stephentoub]

Ok, will close it if there won't be any interest in some short term. Next time will use commits instead if you don't mind.

Thanks. Mainly I want to keep the PR list to things that folks should act on by getting them merged or closed. We're currently pushing 190 PRs, and from I can see, most folks are ignoring anything after the first page or two. I'd love to drive down the number, and getting there means not having unactionable PRs sitting around purely for discussion purposes. For non-committed ideas, I'd like for those discussions to be had on the relevant issues; linking to commits from those issues is a good thing to do, of course.

[CoffeeFlux]

@stephentoub if you to close an issue in dotnet/runtime dealing with Mono code, please try to close the corresponding mono/mono mirrored PR as well to keep things tidy.

[stephentoub]

if you to close an issue in dotnet/runtime dealing with Mono code, please try to close the corresponding mono/mono mirrored PR as well to keep things tidy.

Ok, will do. If the mirror is able to open PRs, it'd be great if it can close them, too.