Open jakobbotsch opened 1 month ago
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch See info in area-owners.md if you want to be subscribed.
Hmm, this really comes down to the implementation of System.Int64.GetHashCode()
that dereferences m_value
twice:
I wonder if we should introduce a 64-bit implementation that only has a single load.
Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.
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?
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?
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
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?
Do you mean the current C# code is potentially not thread-safe?
while this seems like a reliability thing we would want to guarantee?
I don't think we make any guarantees about such operations in the face of concurrent reads/writes / tearing (which presumably is what you're alluding to).
I don't think we make any guarantees about such operations in the face of concurrent reads/writes / tearing (which presumably is what you're alluding to).
Yeah, that's what I was thinking. I agree it's be nice if the JIT optimized this in some way, but I came at this from the perspective of "it's illegal to duplicate loads in the memory model", and this looked to me like illegal behavior from the JIT initially. In reality what looks like a dereference in the C# source code is not actually one.
Legality of the existing C# impl aside, I am curious what would be a proper optimization in JIT, we have a tree like this:
[000003] --C-------- * RETURN int
[000011] ---XG------ \--* XOR int
[000005] ---XG------ +--* CAST int <- long
[000004] ---XG------ | \--* IND long
[000000] ----------- | \--* LCL_VAR byref V01 arg1
[000010] ---XG------ \--* CAST int <- long
[000009] ---XG------ \--* RSH long
[000007] ---XG------ +--* IND long
[000006] ----------- | \--* LCL_VAR byref V01 arg1 (last use)
[000008] ----------- \--* CNS_INT int 32
So far so good, both IND should be CSE'd just fine (without violating our MM), but then morph
kicks in (optNarrowTree
to be precise) and optimized the leading (int)IND<long>
into IND<int>
so we no longer can do any CSE here (unless CSE can do some super-smart alias analysis)
I don't think we make any guarantees about such operations in the face of concurrent reads/writes / tearing (which presumably is what you're alluding to).
I agree with this. At the same time it would be pretty trivial to update the managed code and make this "better" (at least for 64-bit) for the core Int64
/UInt64
types and so it might be nice to do so for .NET 10.
but then morph kicks in (optNarrowTree to be precise) and optimized the leading (int)IND
into IND so we no longer can do any CSE here (unless CSE can do some super-smart alias analysis)
Like I mentioned on Discord, I think this one feels like the cast(ind(x))
shouldn't be done in morph but rather instead in lowering. I'd expect that's "better" for typical user code and that perf oriented code is likely manually doing tricks like hoisting fields/etc so it won't pessimize them
Alternatively, it feels like we might be missing a simpler opt that looks for common aliases from the same address. That is, we're allowed to fold neighboring loads and the like and we already peephole optimize to ldp
on arm64 and similar. So maybe we're missing something that looks for cases like ind32(x)
and ind64(x)
and still allows CSE, for primitive indirections at least (the CSE would hoist to ind64
and then the ind32 becomes a cast from that). That feels like it wouldn't need super-smart alias analysis to be covered and should also handle this scenario
@tannergooding I'm assigning this to you, but I wasn't sure if this is something we'd still consider for .NET 9 or if we should punt it out. If it is something we should do in .NET 9, please coordinate who can take it on and make sure the assignment matches that. Thanks.
This isn't a correctness thing and is relatively low priority, moving to 10
https://github.com/dotnet/runtime/blob/f8bcd05a73e2c102eb940e14bf3f0f9fd23745e5/src/tests/JIT/Regression/JitBlue/GitHub_19149/GitHub_19149.cs#L44-L56
In this test we seem to end up duplicating the loads of
x
: