Set some missing GTF_GLOB_REF and improve its checking.
#61245
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThe code in The diff is a regression but a small one. The diffs look like correctness fixes, for example, for such comparison: we forbid swapping ***** BB01
++STMT00002 (IL ???... ???)
++ [000013] -A--G------- * ASG byref
++ [000012] D------N---- +--* LCL_VAR byref V03 tmp2
++ [000003] ----G------- \--* ADD byref
++ [000001] n---G------- +--* IND ref
++ [000000] I----------- | \--* CNS_INT(h) long 0xd1ffab1e static Fseq[hackishFieldName]
++ [000002] ------------ \--* CNS_INT long 8 Fseq[#FirstElem]
***** BB01
STMT00001 (IL ???... ???)
[000011] I-C-G------- * CALL void System.TimeSpan..ctor (exactContextHnd=0x00000000D1FFAB1E)
[000010] ------------ this in rcx +--* ADDR byref
[000009] -------N---- | \--* LCL_VAR struct<System.TimeSpan, 8> V02 tmp1
[000005] ------------ arg1 \--* CNS_INT long 1
***** BB01
STMT00002 (IL 0x00C... ???)
-- [000013] I-C-G------- * CALL struct System.TimeSpan.Add (exactContextHnd=0x00000000D1FFAB1E)
-- [000003] ------------ this in rcx +--* ADD byref
-- [000001] ------------ | +--* IND ref
-- [000000] I----------- | | \--* CNS_INT(h) long 0xd1ffab1e static Fseq[hackishFieldName]
-- [000002] ------------ | \--* CNS_INT long 8 Fseq[#FirstElem]
-- [000015] n----------- arg1 \--* OBJ struct<System.TimeSpan, 8>
-- [000014] ------------ \--* ADDR byref
-- [000012] -------N---- \--* LCL_VAR struct<System.TimeSpan, 8> V02 tmp1
++STMT00003 (IL 0x00C... ???)
++ [000016] I-C-G------- * CALL struct System.TimeSpan.Add (exactContextHnd=0x00000000D1FFAB1E)
++ [000014] ------------ this in rcx +--* LCL_VAR byref V03 tmp2
++ [000018] n----------- arg1 \--* OBJ struct<System.TimeSpan, 8>
++ [000017] ------------ \--* ADDR byref
++ [000015] -------N---- \--* LCL_VAR struct<System.TimeSpan, 8> V02 tmp1 so if inside then before this change, we would read the changed value, not the correct one. diffs with misleading total numbersIgnore the total diff, see #60124 ``` Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.x64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.x64.checked.4\diff Total bytes of delta: -6956 (-0.10 % of base) 1 total files with Code Size differences (0 improved, 1 regressed), 1 unchanged.Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.x64.checked.5\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.x64.checked.5\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.x64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.x64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.x64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.x64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.Linux.x64.checked.5\base D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.Linux.x64.checked.5\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.x64.checked.5\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.x64.checked.5\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.x64.checked.5\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.x64.checked.5\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.x64.checked.5\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.x64.checked.5\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.arm64.checked.4\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.arm64.checked.4\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.x86.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.benchmarks.run.windows.x86.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.x86.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.windows.x86.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.x86.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.windows.x86.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.x86.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.windows.x86.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.arm.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.coreclr_tests.pmi.Linux.arm.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.arm.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries.pmi.Linux.arm.checked.3\diff Creating dasm files: D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.arm.checked.3\base D:\Sergey\git\runtime\artifacts\spmi\asm.libraries_tests.pmi.Linux.arm.checked.3\diff |
|
Many regressions are caused by the need to spill such reads before inlinings or tail calls, so @jakobbotsch could you please review it? |
Is that a legal thing to do? Since this is a box in which the struct static is stored, it seems like UB if managed code tried to access it in some way. (Edit: it would be semantically equivalent to mutating the static's address) |
| // must have it set as well. However, we clear it from the parent in cases | ||
| // ADDR(LCL_VAR or CLS_VAR). | ||
| if (oper == GT_ADDR && (op1->OperIs(GT_LCL_VAR, GT_CLS_VAR) || | ||
| (op1->OperIs(GT_IND) && op1->AsOp()->gtOp1->OperIs(GT_CLS_VAR_ADDR)))) |
There was a problem hiding this comment.
Is OperIs(GT_CLS_VAR_ADDR) ever true here? AFAIK we only use those opers very late.
There was a problem hiding this comment.
you are right, I was adding it to sync with fgDebugCheckFlags but the only moment when we need it after rationalizing (created first GT_CLS_VAR_ADDR) and before lowering (don't call fgDebugCheckStmtsList->fgDebugCheckFlags in LIR). I will delete this change and add a not to why we need it in fgDebugCheckFlags, thanks for catching this.
I am not sure, probably @AndyAyersMS could help here. gives us the address of the field and so it can't change from managed code? Would it be right then to mark it as an invariant (it still can change due to GC, cant it?) and drop global? |
Right. I guess technically it gives us "the base", we will also add an offset to it to get to the actual data, and that will be the value private static Vector128<int> _vtor;
[MethodImpl(MethodImplOptions.NoInlining)]
private static ref Vector128<int> AddressOfStructStatic()
{
return ref _vtor;
}N005 ( 7, 15) [000004] ------------ * RETURN byref
N004 ( 6, 14) [000003] ------------ \--* ADD byref
N002 ( 4, 12) [000001] n----------- +--* IND ref
N001 ( 2, 10) [000000] I----------- | \--* CNS_INT(h) long 0xd1ffab1e static Fseq[_vtor]
N003 ( 1, 1) [000002] ------------ \--* CNS_INT long 8 Fseq[#FirstElem]I don't think there is a combination of IL instructions that will allow one to get to the underlying box and mutate the reference itself.
I tried recently to mark it as invariant, but static readonly Vector128<float> _vtor = Vector128.Create(1.0f, 2.0f, 3.0f, 4.0f);
static int _justSomeField;
private static Vector128<float> Vtors()
{
var a = _vtor;
_justSomeField = 1;
for (int i = 0; i < 10; i++)
{
a = Sse.Add(a, _vtor);
}
return a;
}- x86 codegen for the loop: G_M54979_IG03:
vaddps xmm1, xmm1, xmm0
inc eax
cmp eax, 10
jl SHORT G_M54979_IG03IR in VN: ***** BB02, STMT00004(before)
N008 ( 19, 16) [000025] -A-XG---R--- * ASG simd16 (copy)
N007 ( 3, 2) [000023] D------N---- +--* LCL_VAR simd16<System.Runtime.Intrinsics.Vector128`1[Single]> V01 loc0 d:5
N006 ( 15, 13) [000022] ---XG------- \--* HWINTRINSIC simd16 float Add
N001 ( 3, 2) [000017] ------------ +--* LCL_VAR simd16<System.Runtime.Intrinsics.Vector128`1[Single]> V01 loc0 u:4 (last use)
N005 ( 11, 10) [000021] ---XG------- \--* OBJ simd16<System.Runtime.Intrinsics.Vector128`1[Single]>
N004 ( 5, 6) [000020] ----G------- \--* ADD byref
N002 ( 3, 4) [000018] ----G------- +--* CLS_VAR ref Hnd=0xd1ffab1e Fseq[_vtor]
N003 ( 1, 1) [000019] ------------ \--* CNS_INT int 4 Fseq[#FirstElem]
N001 [000017] LCL_VAR V01 loc0 u:4 (last use) => $2c0 {PhiDef($1, $4, $1c3)}
FieldSeq {_vtor} is $180
N002 [000018] CLS_VAR Hnd=0xd1ffab1e Fseq[_vtor] => $81 {PtrToStatic($180)}
N003 [000019] CNS_INT 4 Fseq[#FirstElem] => $42 {IntCns 4}
FieldSeq {#FirstElem} is $181
fieldSeq $182 is {(_vtor, #FirstElem)}
N004 [000020] ADD => $82 {PtrToStatic($182)}
Known type Vector128<float>
VNApplySelectors:
VNForHandle(_vtor) is $140, fieldType is simd16, size = 16
AX2: $140 != $142 ==> select([$240]store($100, $142, $43), $140) ==> select($100, $140) remaining budget is 99.
VNForMapSelect($240, $140):simd16 returns $1c0 {$100[$140]}
N005 [000021] OBJ => <l:$1c2 {norm=$1c0 {$100[$140]}, exc=$183 {NullPtrExc($82)}}, c:$1c4 {norm=$202 {MemOpaque:L00}, exc=$183 {NullPtrExc($82)}}>
N006 [000022] HWINTRINSIC => <l:$1c8 {norm=$1c6 {HWI_SSE_Add($2c0, $1c0)}, exc=$183 {NullPtrExc($82)}}, c:$1c7 {norm=$1c5 {HWI_SSE_Add($2c0, $202)}, exc=$183 {NullPtrExc($82)}}>
N007 [000023] LCL_VAR V01 loc0 d:5 => <l:$1c6 {HWI_SSE_Add($2c0, $1c0)}, c:$1c5 {HWI_SSE_Add($2c0, $202)}>
N008 [000025] ASG => <l:$1c8 {norm=$1c6 {HWI_SSE_Add($2c0, $1c0)}, exc=$183 {NullPtrExc($82)}}, c:$1c7 {norm=$1c5 {HWI_SSE_Add($2c0, $202)}, exc=$183 {NullPtrExc($82)}}>(We label it as |
|
Very interesting,
the only scenario that I can imagine is GC.collect but then its VM responsibility to update the result of the
we can try to combine these fixes for |
|
It would be nice to separate out refactorings/cleanups (like wider use of As others have noted, access to some statics might require more than one indir; I believe all save the outermost should be invariant and non-faulting. |
sure, the refactorings came last moment when I was analyzing regressions and was surprised that the flags are not shown under the debugger, I will extract this change. |
I did not do much - just marked the address indirection as invariant. The problem was that the handle under it implies a mutable static, so that's why the checker was complaining. I then (after the replays came back red) solved the problem that prompted to me to look into this in the first place in a different way. |
|
@sandreenko is this ready for re-review? |
no, looks like we need to understand when to set |
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
The code in
fgDebugCheckFlagshad some mistakes that were easy to fix, for harder errors I added a comment explaining them.The diff is a regression but a small one.
The diffs look like correctness fixes, for example, for such comparison:
we forbid swapping
callandIND(global static field), if I understand correctly, nothing prevents the call from making a change in this field, so it should not be hard to create a correctness repro for this bug.so if inside
[000011] I-C-G------- * CALL void System.TimeSpan..ctorwe change what is stored inthen before this change, we would read the changed value, not the correct one.
diffs with misleading total numbers
Ignore the total diff, see #60124