JIT: allow slightly more general promotion of structs with struct fields #22867
Conversation
For a while now the jit has been able to promote an outer struct A with an inner struct field B that itself has a single non-struct field C, provided that C occupies all of B and that C and B are pointer-sized. For example, this comes up when supporting promotion of `Span<T>`, as a span contains a `ByReference<T>` field that itself contains a pointer-sized field. This change relaxes the constraints slightly, allowing B and C to be less than pointer sized, provided C still occupies all of B, and B is suitably aligned within A. Doing so allows promotion of the new `Range` type, which contains two `Index` fields that each wrap an `int`. This improves performance for uses of `Range` for simple examples like those in #22079.
|
cc @dotnet/jit-contrib Will keep #22079 open after this, but move remainder of work to future. FX diff impact: Test case impact: [MethodImpl(MethodImplOptions.NoInlining)]
static Span<int> TrimFirstLast_OpenCoded(Span<int> s) => s.Slice(1, s.Length - 1);
[MethodImpl(MethodImplOptions.NoInlining)]
static Span<int> TrimFirstLast_Range(Span<int> s) => s[new Range(Index.FromStart(1), Index.FromEnd(0))]; |
|
Perf numbers. Still a big gap, but better than it was. Notes explaining why over in #22079.
|
|
Looks like this hit a whole raft of jenkins hangups. |
|
@dotnet-bot test Windows_NT x86 Release Innerloop Build and Test |
|
More hangups... @dotnet-bot test Windows_NT x64 Checked CoreFX Tests Release x64 CoreFX test may be real, will need to investigate: |
|
LGTM |
|
Yes, it handles bytes. But there is something odd going on in VN that I want to look into further. Example: using System;
struct B
{
public byte x;
}
struct BB
{
public B b1;
public B b2;
}
class X
{
public static int Main()
{
BB bb = new BB();
bb.b1.x = 64;
bb.b2.x = 36;
return (int) bb.b1.x + (int) bb.b2.x;
}
};; before
;; Promotion blocked: struct contains struct field with one field, but that field has invalid size or type
...
mov eax, 100
ret
;; after
;; Promoting struct local V00 (BB):
;; lvaGrabTemp returning 2 (V02 tmp1) (a long lifetime temp) called for field V00.b1 (fldOffset=0x0).
;; lvaGrabTemp returning 3 (V03 tmp2) (a long lifetime temp) called for field V00.b2 (fldOffset=0x1).
...
push rax
nop
mov byte ptr [rsp+04H], 64
mov byte ptr [rsp], 36
movzx rax, byte ptr [rsp+04H]
movzx rdx, byte ptr [rsp]
add eax, edx
add rsp, 8
ret
For some reason VN is unable to propagate constants though the promoted fields. |
|
Some notes on the byte wrapper example. First, in morph.... in the after case, we have: Where Morph transforms the RHS into and then tries to simplify the Because morph sees a small type and a normalize on load temp, it won't fold this to just But here the IR is storing to Later on at the return we do hit uses and here the normalize on load check behaves similarly. But if we allowed folding in that case it looks like we'd go on to call So I'm wondering if we can just remove those normalize on load blockers entirely, or if not, put in a more accurate check. None of this should be needed to enable const prop later on, so will look into that next. |
|
Ahm, promotion of small int fields, be careful with that because the JIT does something weird: https://github.com/dotnet/coreclr/issues/20957#issuecomment-442171482. It also results in other CQ isuess: https://github.com/dotnet/coreclr/issues/20957#issuecomment-445941114 |
|
Second, for value numbering: When VN encounters the write to a GT_LCL_FLD, since it is an entire write, it simply uses the value number of the source. But for reads of GT_LCL_FLD, VN always goes via the field selectors: So the read doesn't pick up the constant. In the "before" case none of the accesses are entire, things match up, and constants propagate. Seems like we need to duplicate the "entire" logic here and just use the SSA value number for the local. |
|
Am going to split off the VN change as a separate PR. Will leave morph alone for now. |
|
Regression in |
|
The CoreFX failures (release, checked) seem somewhat random. Let's see if they repro. @dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
|
Got some different checked CoreFX failures this time. |
|
Ubuntu arm stuff never ran @dotnet-bot test Ubuntu arm Cross Checked Innerloop Build and Test |
|
And will take one more shot at checked CoreFX... @dotnet-bot test Windows_NT x64 Checked CoreFX Tests |
|
Not a fan of rerunning tests until they pass, but I am fairly sure the CoreFX failures seen above were unrelated. |
Fixes an oversight in dotnet#22867.
Fixes an oversight in #22867.
…lds (dotnet/coreclr#22867) For a while now the jit has been able to promote an outer struct A with an inner struct field B that itself has a single non-struct field C, provided that C occupies all of B and that C and B are pointer-sized. For example, this comes up when supporting promotion of `Span<T>`, as a span contains a `ByReference<T>` field that itself contains a pointer-sized field. This change relaxes the constraints slightly, allowing B and C to be less than pointer sized, provided C still occupies all of B, and B is suitably aligned within A. Doing so allows promotion of the new `Range` type, which contains two `Index` fields that each wrap an `int`. This improves performance for uses of `Range` for simple examples like those in dotnet/coreclr#22079. Commit migrated from dotnet/coreclr@8f5bf71
…/coreclr#22952) Fixes an oversight in dotnet/coreclr#22867. Commit migrated from dotnet/coreclr@bd3a20d
For a while now the jit has been able to promote an outer struct A with an
inner struct field B that itself has a single non-struct field C, provided
that C occupies all of B and that C and B are pointer-sized.
For example, this comes up when supporting promotion of
Span<T>, as a spancontains a
ByReference<T>field that itself contains a pointer-sized field.This change relaxes the constraints slightly, allowing B and C to be less than
pointer sized, provided C still occupies all of B, and B is suitably aligned
within A.
Doing so allows promotion of the new
Rangetype, which contains twoIndexfields that each wrap an
int. This improves performance for uses ofRangefor simple examples like those in #22079.