-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Description
It looks like some of the jit optimizations aren't String.Empty friendly and prefer "" instead, e.g.:
Inliner
class Tests
{
static int GetLengthWithLimit(string str) => str.Length > 10 ? str.Length : 0;
// I expect both Case1 and Case2 to have the same codegen:
int Case1() => GetLengthWithLimit("");
int Case2() => GetLengthWithLimit(string.Empty);
}Current codegen for Case1 and Case2:
; Method Case1():int:this
33C0 xor eax, eax ;; <--- inlined & folded into `return 0;`
C3 ret
; Total bytes of code: 3
; Method Case2():int:this
48B9603000E9D7010000 mov rcx, 0x1D7E9003060
488B09 mov rcx, gword ptr [rcx]
E95E81FFFF jmp Tests:GetLengthWithLimit(System.String):int ;; <--- not even inlined!
; Total bytes of code: 18JIT imports string.Empty as a GT_IND + <address> node whether "" is emitted as GT_CNS_STR (which is then transformed into GT_IND + <address> later but the optimizations expect GT_CNS_STR). Since GT_CNS_STR node is know to be a const (has GTK_CONST flag), the inliner for the second case (Case2) has:
Inline candidate has **const arg** that feeds a conditional. Multiplier increased to 5.
and doesn't have it for Case1.
String.get_Length optimization
This optimization also supports only GT_CNS_STR nodes, e.g:
int Case1() => "".Length;
int Case2() => string.Empty.Length;Codgen:
; Method Case1():int:this
33C0 xor eax, eax
C3 ret
; Method Case2():int:this
488B00 mov rax, gword ptr [rax]
8B4008 mov eax, dword ptr [rax+8]
C3 ret I wonder if we can easily fix it by importing String.Empty as a fake (or real) GT_CNS_STR node like this: EgorBo@09f81b1 (this prototype fixes both issues above ^)
Jit-diff: https://gist.github.com/EgorBo/c6c12e79b7b90ff7aaee58f3fb5a49fb (diff is a "regression" because the fix allows to inline more stuff)
/cc @dotnet/jit-contrib
category:cq
theme:basic-cq
skill-level:beginner
cost:small