R2R fixes for [Equality]Comparer.Default in jit

[EgorBo]

bool Foo(int a, int b) => EqualityComparer<int>.Default.Equals(a, b);

Current R2R codegen:

; Method Foo(int,int):bool:this
G_M42273_IG01:              ;; offset=0000H
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       8BF2                 mov      esi, edx
       418BF8               mov      edi, r8d
G_M42273_IG02:              ;; offset=000BH
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       488B00               mov      rax, gword ptr [rax]
       3800                 cmp      byte  ptr [rax], al
       33C0                 xor      eax, eax
       3BF7                 cmp      esi, edi
       0F94C0               sete     al
G_M42273_IG03:              ;; offset=001DH
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code: 36

New R2R codegen:

; Method Foo(int,int):bool:this
G_M42273_IG01:              ;; offset=0000H
G_M42273_IG02:              ;; offset=0000H
       33C0                 xor      eax, eax
       413BD0               cmp      edx, r8d
       0F94C0               sete     al
G_M42273_IG03:              ;; offset=0008H
       C3                   ret    
; Total bytes of code: 9

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

bool Foo(int a, int b) => EqualityComparer<int>.Default.Equals(a, b);

Current R2R codegen:

; Method Foo(int,int):bool:this
G_M42273_IG01:              ;; offset=0000H
       57                   push     rdi
       56                   push     rsi
       4883EC28             sub      rsp, 40
       8BF2                 mov      esi, edx
       418BF8               mov      edi, r8d
G_M42273_IG02:              ;; offset=000BH
       FF1500000000         call     [CORINFO_HELP_READYTORUN_STATIC_BASE]
       488B00               mov      rax, gword ptr [rax]
       3800                 cmp      byte  ptr [rax], al
       33C0                 xor      eax, eax
       3BF7                 cmp      esi, edi
       0F94C0               sete     al
G_M42273_IG03:              ;; offset=001DH
       4883C428             add      rsp, 40
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      
; Total bytes of code: 36

New R2R codegen:

; Method Foo(int,int):bool:this
G_M42273_IG01:              ;; offset=0000H
G_M42273_IG02:              ;; offset=0000H
       33C0                 xor      eax, eax
       413BD0               cmp      edx, r8d
       0F94C0               sete     al
G_M42273_IG03:              ;; offset=0008H
       C3                   ret    
; Total bytes of code: 9

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

/azp run runtime-coreclr r2r, runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[AndyAyersMS]

This optimization worked for R2R at one time, eg

runtime/src/coreclr/jit/fginline.cpp

Lines 1616 to 1673 in b1edd42

	// In some special cases, unused args with side effects can
	// trigger further changes.
	//
	// (1) If the arg is a static field access and the field access
	// was produced by a call to EqualityComparer<T>.get_Default, the
	// helper call to ensure the field has a value can be suppressed.
	// This helper call is marked as a "Special DCE" helper during
	// importation, over in fgGetStaticsCCtorHelper.
	//
	// (2) NYI. If we find that the actual arg expression
	// has no side effects, we can skip appending all
	// together. This will help jit TP a bit.
	//
	assert(!argNode->OperIs(GT_RET_EXPR));
	// For case (1)
	//
	// Look for the following tree shapes
	// prejit: (IND (ADD (CONST, CALL(special dce helper...))))
	// jit : (COMMA (CALL(special dce helper...), (FIELD ...)))
	if (argNode->gtOper == GT_COMMA)
	{
	// Look for (COMMA (CALL(special dce helper...), (FIELD ...)))
	GenTree* op1 = argNode->AsOp()->gtOp1;
	GenTree* op2 = argNode->AsOp()->gtOp2;
	if (op1->IsCall() &&
	((op1->AsCall()->gtCallMoreFlags & GTF_CALL_M_HELPER_SPECIAL_DCE) != 0) &&
	(op2->gtOper == GT_FIELD) && ((op2->gtFlags & GTF_EXCEPT) == 0))
	{
	JITDUMP("\nPerforming special dce on unused arg [%06u]:"
	" actual arg [%06u] helper call [%06u]\n",
	argNode->gtTreeID, argNode->gtTreeID, op1->gtTreeID);
	// Drop the whole tree
	append = false;
	}
	}
	else if (argNode->gtOper == GT_IND)
	{
	// Look for (IND (ADD (CONST, CALL(special dce helper...))))
	GenTree* addr = argNode->AsOp()->gtOp1;
	if (addr->gtOper == GT_ADD)
	{
	GenTree* op1 = addr->AsOp()->gtOp1;
	GenTree* op2 = addr->AsOp()->gtOp2;
	if (op1->IsCall() &&
	((op1->AsCall()->gtCallMoreFlags & GTF_CALL_M_HELPER_SPECIAL_DCE) != 0) &&
	op2->IsCnsIntOrI())
	{
	// Drop the whole tree
	JITDUMP("\nPerforming special dce on unused arg [%06u]:"
	" actual arg [%06u] helper call [%06u]\n",
	argNode->gtTreeID, argNode->gtTreeID, op1->gtTreeID);
	append = false;
	}
	}
	}
	}

What changed? Why did it break?

[EgorBo]

So my PR does two things:

1. It avoid marking call as null-check required, so get_Default won't be used by GT_NULLCHECK and will be removable
2. flags |= GTF_CALL_M_HELPER_SPECIAL_DCE was never hit by R2R because of this block https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/importer.cpp#L9021-L9037

[AndyAyersMS]

Ok. The initial work here (dotnet/coreclr#14125) was done in 2.1, before we enabled R2R in 3.0, so the support noted above was for fragile ngen. Can we remove that bit of logic or is it still needed for R2R?

[AndyAyersMS]

Would be nice to re-use the pattern in fgIsCurrentCallDceCandidate here -- maybe generalize that to take a method handle?

[EgorBo]

Technically this is a bit different logic, it's a quick check that we can skip a nullcheck, we can add here more intrinsics, potentially:

NI_System_Threading_Thread_get_CurrentThread
NI_System_Array_Clone
NI_System_Object_MemberwiseClone
NI_System_Object_GetType

all of these are not expected to return null afair

[AndyAyersMS]

Got it.

We should perhaps create a class similar to HelperCallProperties where we can capture this sort of thing in one place; it may be useful to know elsewhere.

[EgorBo]

Something changed in Main and this no longer works, will re-open & address once have time