Should `System.Int64.GetHashCode()` avoid dereferencing `m_value` twice?

[jakobbotsch]

runtime/src/tests/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs

Lines 44 to 56 in f8bcd05

	public override int GetHashCode()
	{
	fixed (long* lPtr = _chunks)
	{
	var hashCode = -1923861349;
	long* x = lPtr;
	for (int i = 0; i < ChunkLength; i++)
	{
	hashCode = hashCode * -1521134295 + (*x++).GetHashCode();
	}
	return hashCode;
	}
	}

In this test we seem to end up duplicating the loads of x:

; Assembly listing for method CommandBytes:GetHashCode():int:this (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T02] (  5,  5   )   byref  ->  rcx         this single-def
;  V01 loc0         [V01    ] (  1,  1   )   byref  ->  [rsp+0x00]  pinned single-def
;  V02 loc1         [V02,T01] (  6,  6   )     int  ->  rcx
;  V03 loc2         [V03,T03] (  6,  6   )    long  ->  registers
;* V04 loc3         [V04,T05] (  0,  0   )     int  ->  zero-ref
;# V05 OutArgs      [V05    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;  V06 tmp1         [V06,T00] ( 11, 22   )    long  ->  registers   "impSpillLclRefs"
;  V07 tmp2         [V07,T04] (  2,  4   )    long  ->  rcx         "Cast away GC"
;
; Lcl frame size = 8

G_M45379_IG01:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, nogc <-- Prolog IG
       push     rax
                                                ;; size=1 bbWeight=1 PerfScore 1.00
G_M45379_IG02:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0002 {rcx}, byref
                            ; byrRegs +[rcx]
       cmp      byte  ptr [rcx], cl
       mov      bword ptr [rsp], rcx
       lea      rax, [rcx+0x08]
       mov      edx, dword ptr [rcx]
       mov      rcx, qword ptr [rcx]  ; duplicated load
                            ; byrRegs -[rcx]
       sar      rcx, 32
       xor      ecx, edx
       add      ecx, 0xFFFFFFFFF66AE3D3
       lea      rdx, [rax+0x08]
       mov      r8d, dword ptr [rax]
       mov      rax, qword ptr [rax]
       sar      rax, 32
       xor      eax, r8d
       imul     ecx, ecx, 0xFFFFFFFFA5555529
       add      ecx, eax
       mov      rax, rdx
       mov      edx, dword ptr [rax]
       mov      rax, qword ptr [rax]
       sar      rax, 32
       xor      eax, edx
       imul     ecx, ecx, 0xFFFFFFFFA5555529
       add      ecx, eax
       mov      eax, ecx
                                                ;; size=76 bbWeight=1 PerfScore 24.50
G_M45379_IG03:        ; bbWeight=1, epilog, nogc, extend
       add      rsp, 8
       ret
                                                ;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 82, prolog size 1, PerfScore 26.75, instruction count 26, allocated bytes for code 82 (MethodHash=fdc84ebc) for method CommandBytes:GetHashCode():int:this (FullOpts)
; ============================================================

[dotnet-policy-service]

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

[jakobbotsch]

Hmm, this really comes down to the implementation of System.Int64.GetHashCode() that dereferences m_value twice:

runtime/src/libraries/System.Private.CoreLib/src/System/Int64.cs

Lines 106 to 109 in f8bcd05

	public override int GetHashCode()
	{
	return unchecked((int)((long)m_value)) ^ (int)(m_value >> 32);
	}

I wonder if we should introduce a 64-bit implementation that only has a single load.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

[EgorBo]

Hmm, this really comes down to the implementation of System.Int64.GetHashCode() that dereferences m_value twice:

@jakobbotsch Other methods like CompareTo, etc and in all other primitives we do that. Why don't these load get the same VN?

[stephentoub]

I wonder if we should introduce a 64-bit implementation that only has a single load.

Meaning just changing the impl to be:

 public override int GetHashCode() 
 { 
     long value = m_value;
     return unchecked((int)value ^ (int)(value >> 32); 
 } 

? We certainly can, but it'd be nice if it weren't necessary. The value isn't volatile; why can't the JIT eliminate the duplicate load?

[EgorBo]

I guess the minimal repro is:

int Test(long* ptr)
{
    return (*ptr).GetHashCode();
}

       mov      eax, dword ptr [rdx]
       mov      rcx, qword ptr [rdx]
       sar      rcx, 32
       xor      eax, ecx

[jakobbotsch]

I wonder if we should introduce a 64-bit implementation that only has a single load.

Meaning just changing the impl to be:

 public override int GetHashCode() 
 { 
     long value = m_value;
     return unchecked((int)value ^ (int)(value >> 32); 
 } 

Yeah, something like that.

@jakobbotsch Other methods like CompareTo, etc and in all other primitives we do that. Why don't these load get the same VN?
? It'd be nice if it weren't necessary. The value isn't volatile; why can't the JIT eliminate the duplicate load?

Most likely because we optimize one of the indirection to a 32-bit indirection, and then we aren't
able to collapse them.

Even if we did collapse them that optimization would likely only happen in Tier-1, while this seems like a reliability thing we would want to guarantee?

[EgorBo]

Do you mean the current C# code is potentially not thread-safe?