From 13344be7d6950e2678e5b79a63357f0794ef70a4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 10 Jan 2023 14:39:36 +0100 Subject: [PATCH 01/17] Fold string fields in structs located in static readonly fields --- src/coreclr/jit/valuenum.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 8e2c86b2d73c..d2db78b14349 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5126,6 +5126,31 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel { noway_assert(fieldSeq != nullptr); + // Check if the load represents a frozen gc object located in a struct which is stored in a static readonly field, + // e.g.: + // + // struct MyStruct { string Name; } + // static readonly MyStruct MyStr = new() { Name = "Hey!" }; + // + // string GetName() => MyStr.Name; // <- loadTree + // + if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF)) + { + uint8_t buffer[TARGET_POINTER_SIZE] = {0}; + if (((UINT)offset < INT_MAX) && + info.compCompHnd->getReadonlyStaticFieldValue(fieldSeq->GetFieldHandle(), buffer, TARGET_POINTER_SIZE, + (int)offset)) + { + // In case of 64bit jit emitting 32bit codegen this handle will be 64bit + // value holding 32bit handle with upper half zeroed (hence, "= NULL"). + // It's done to match the current crossgen/ILC behavior. + ssize_t objHandle = NULL; + memcpy(&objHandle, buffer, TARGET_POINTER_SIZE); + loadTree->gtVNPair.SetBoth(vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); + return; + } + } + // Two cases: // // 1) Instance field / "complex" static: heap[field][baseAddr][offset + load size]. From b7c0918ff1e8edc5429944f922faea97ee086cc1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 10 Jan 2023 14:55:14 +0100 Subject: [PATCH 02/17] Add more checks --- src/coreclr/jit/valuenum.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d2db78b14349..a48339e75d8e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5134,7 +5134,8 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel // // string GetName() => MyStr.Name; // <- loadTree // - if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF)) + if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && + (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && (fieldSeq->GetOffset() == TARGET_POINTER_SIZE)) { uint8_t buffer[TARGET_POINTER_SIZE] = {0}; if (((UINT)offset < INT_MAX) && From 5cacf2cc86cddad3843d3b4963fd9071138a60b2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 10 Jan 2023 14:58:12 +0100 Subject: [PATCH 03/17] Add comments --- src/coreclr/jit/valuenum.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a48339e75d8e..c7ddecea3222 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5135,7 +5135,10 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel // string GetName() => MyStr.Name; // <- loadTree // if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && - (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && (fieldSeq->GetOffset() == TARGET_POINTER_SIZE)) + // it should be a static field + (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && + // boxed static to be precise + (fieldSeq->GetOffset() == TARGET_POINTER_SIZE)) { uint8_t buffer[TARGET_POINTER_SIZE] = {0}; if (((UINT)offset < INT_MAX) && From 2269fde5668280ece482426a1636e05bd6abc4a9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 10 Jan 2023 16:57:58 +0100 Subject: [PATCH 04/17] handle null --- src/coreclr/jit/valuenum.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c7ddecea3222..f584973293d6 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5148,9 +5148,10 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel // In case of 64bit jit emitting 32bit codegen this handle will be 64bit // value holding 32bit handle with upper half zeroed (hence, "= NULL"). // It's done to match the current crossgen/ILC behavior. - ssize_t objHandle = NULL; + ssize_t objHandle = 0; memcpy(&objHandle, buffer, TARGET_POINTER_SIZE); - loadTree->gtVNPair.SetBoth(vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); + loadTree->gtVNPair.SetBoth(objHandle == 0 ? vnStore->VNForNull() + : vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); return; } } From 0cc917d6c83ab4e38a78abd8a8dace8010b709d1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 10 Jan 2023 19:42:37 +0100 Subject: [PATCH 05/17] fix assert --- src/coreclr/jit/valuenum.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index f584973293d6..bb44020fff0c 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5150,8 +5150,15 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel // It's done to match the current crossgen/ILC behavior. ssize_t objHandle = 0; memcpy(&objHandle, buffer, TARGET_POINTER_SIZE); - loadTree->gtVNPair.SetBoth(objHandle == 0 ? vnStore->VNForNull() - : vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); + if (objHandle == 0) + { + loadTree->gtVNPair.SetBoth(vnStore->VNForNull()); + } + else + { + loadTree->gtVNPair.SetBoth(vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); + setMethodHasFrozenObjects(); + } return; } } From 9995febb950b494546d7ab26eda0ec62501f9314 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 11 Jan 2023 02:03:25 +0100 Subject: [PATCH 06/17] Fix getReadonlyStaticFieldValue for structs with gc fields --- src/coreclr/vm/jitinterface.cpp | 41 +++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 95c6d41a64fd..94989de2c54f 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11723,8 +11723,45 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t if (size >= (UINT)bufferSize && valueOffset >= 0 && (UINT)valueOffset <= size - (UINT)bufferSize) { - memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize); - result = true; + // For structs containing GC pointers we want to make sure those GC pointers belong to FOH + // so we expect valueOffset to be a real field offset (same for bufferSize) + if (!field->IsRVA() && (field->GetFieldType() == ELEMENT_TYPE_VALUETYPE)) + { + PTR_MethodTable structType = field->GetFieldTypeHandleThrowing().GetMethodTable(); + if (structType->ContainsPointers()) + { + for (WORD i = 0; i < structType->GetNumInstanceFields(); i++) + { + FieldDesc* subField = (FieldDesc*)((structType->GetApproxFieldDescListRaw()) + i); + // TODO: If subField is also a struct we might want to inspect its fields too + if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && subField->IsObjRef()) + { + GCX_COOP(); + Object* subFieldValue = nullptr; + memcpy(&subFieldValue, (uint8_t*)baseAddr + valueOffset, bufferSize); + if (subFieldValue == nullptr || GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(subFieldValue)) + { + // GC handle from FOH or null + memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize); + result = true; + } + break; + } + } + } + else + { + // No gc pointers in the struct + memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize); + result = true; + } + } + else + { + // Primitive or RVA + memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize); + result = true; + } } } } From 53a8e738572e87268f18c30ef9c4ab431a56aa35 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 11 Jan 2023 18:03:19 +0100 Subject: [PATCH 07/17] Update jitinterface.cpp --- src/coreclr/vm/jitinterface.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 94989de2c54f..36261725a64a 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11725,7 +11725,7 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t { // For structs containing GC pointers we want to make sure those GC pointers belong to FOH // so we expect valueOffset to be a real field offset (same for bufferSize) - if (!field->IsRVA() && (field->GetFieldType() == ELEMENT_TYPE_VALUETYPE)) + if (!field->IsRVA() && field->GetFieldType() == ELEMENT_TYPE_VALUETYPE) { PTR_MethodTable structType = field->GetFieldTypeHandleThrowing().GetMethodTable(); if (structType->ContainsPointers()) @@ -11734,7 +11734,8 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t { FieldDesc* subField = (FieldDesc*)((structType->GetApproxFieldDescListRaw()) + i); // TODO: If subField is also a struct we might want to inspect its fields too - if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && subField->IsObjRef()) + if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && subField->IsObjRef() && + subField->GetFieldType() != ELEMENT_TYPE_VALUETYPE) { GCX_COOP(); Object* subFieldValue = nullptr; @@ -11747,6 +11748,11 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t } break; } + + if (subField->GetOffset() >= (DWORD)valueOffset) + { + break; + } } } else From 53716c85e56c823f400ee8f6880804a911b70671 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 11 Jan 2023 22:16:59 +0100 Subject: [PATCH 08/17] Address feedback --- src/coreclr/vm/jitinterface.cpp | 35 +++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 36261725a64a..96101f5317c7 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11727,30 +11727,39 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t // so we expect valueOffset to be a real field offset (same for bufferSize) if (!field->IsRVA() && field->GetFieldType() == ELEMENT_TYPE_VALUETYPE) { - PTR_MethodTable structType = field->GetFieldTypeHandleThrowing().GetMethodTable(); + PTR_MethodTable structType = field->GetFieldTypeHandleThrowing().AsMethodTable(); if (structType->ContainsPointers()) { - for (WORD i = 0; i < structType->GetNumInstanceFields(); i++) + ApproxFieldDescIterator fieldIterator(structType, ApproxFieldDescIterator::INSTANCE_FIELDS); + for (FieldDesc* subField = fieldIterator.Next(); subField != NULL; subField = fieldIterator.Next()) { - FieldDesc* subField = (FieldDesc*)((structType->GetApproxFieldDescListRaw()) + i); // TODO: If subField is also a struct we might want to inspect its fields too - if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && subField->IsObjRef() && - subField->GetFieldType() != ELEMENT_TYPE_VALUETYPE) + if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && + subField->IsObjRef() && subField->GetFieldType() != ELEMENT_TYPE_VALUETYPE) { GCX_COOP(); + + // Read field's value Object* subFieldValue = nullptr; memcpy(&subFieldValue, (uint8_t*)baseAddr + valueOffset, bufferSize); - if (subFieldValue == nullptr || GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(subFieldValue)) + + if (subFieldValue == nullptr) { - // GC handle from FOH or null - memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize); + // Report null + memset(buffer, 0, bufferSize); result = true; } - break; - } - - if (subField->GetOffset() >= (DWORD)valueOffset) - { + else if (GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(subFieldValue)) + { + CORINFO_OBJECT_HANDLE handle = getJitHandleForObject( + ObjectToOBJECTREF(subFieldValue), /*knownFrozen*/ true); + + // GC handle is either from FOH or null + memcpy(buffer, &handle, bufferSize); + result = true; + } + + // We're done with this struct break; } } From feced0f008fda92a2028ecadc7c948d4b1a7028a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 11 Jan 2023 22:33:59 +0100 Subject: [PATCH 09/17] Add tests --- .../StaticReadonlyStructWithGC.cs | 76 +++++++++++++++++++ .../StaticReadonlyStructWithGC.csproj | 9 +++ 2 files changed, 85 insertions(+) create mode 100644 src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs create mode 100644 src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj diff --git a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs new file mode 100644 index 000000000000..852906bbc3f2 --- /dev/null +++ b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs @@ -0,0 +1,76 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; + +class StaticReadonlyStructWithGC +{ + static int Main() + { + for (int i = 0; i < 100; i++) + { + if (!Test1()) throw new Exception("Test1 failed"); + if (!Test2()) throw new Exception("Test2 failed"); + if (!Test3()) throw new Exception("Test3 failed"); + if (!Test4()) throw new Exception("Test4 failed"); + if (!Test5()) throw new Exception("Test5 failed"); + if (!Test6()) throw new Exception("Test6 failed"); + if (!Test7()) throw new Exception("Test7 failed"); + if (!Test8()) throw new Exception("Test8 failed"); + if (!Test9()) throw new Exception("Test9 failed"); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + Thread.Sleep(16); + } + return 100; + } + + static readonly MyStruct MyStructFld = new() + { + A = "A", + B = 111111.ToString(), // non-literal + C = new MyStruct2 { A = "AA" }, + D = typeof(int), + E = () => 42, + F = new MyStruct3 { A = typeof(double), B = typeof(string) }, + G = new int[0], + H = null + }; + + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test1() => MyStructFld.A == "A"; + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test2() => MyStructFld.B == "111111"; + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test3() => MyStructFld.C.A == "AA"; + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test4() => MyStructFld.D == typeof(int); + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test5() => MyStructFld.E() == 42; + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test6() => MyStructFld.F.A == typeof(double); + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test7() => MyStructFld.F.B == typeof(string); + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test8() => MyStructFld.G.Length == 0; + [MethodImpl(MethodImplOptions.NoInlining)] static bool Test9() => MyStructFld.H == null; + + struct MyStruct + { + public string A; + public string B; + public MyStruct2 C; + public Type D; + public Func E; + public MyStruct3 F; + public int[] G; + public object H; + } + + struct MyStruct2 + { + public string A; + } + + struct MyStruct3 + { + public Type A; + public Type B; + } +} diff --git a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj new file mode 100644 index 000000000000..6946bed81bfd --- /dev/null +++ b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From c0b6956bafb8e5d5ca8c9e878a168b285ea61d97 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 11 Jan 2023 22:34:45 +0100 Subject: [PATCH 10/17] the test needs tiering --- .../JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj index 6946bed81bfd..9d5af3bce3d8 100644 --- a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj +++ b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj @@ -5,5 +5,6 @@ + From 278c35bf0370fff1359eb206c5202fd0aae8a8c0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Mon, 6 Mar 2023 19:44:41 +0100 Subject: [PATCH 11/17] Update src/coreclr/vm/jitinterface.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/jitinterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 96101f5317c7..f0c8354fad77 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11735,7 +11735,7 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t { // TODO: If subField is also a struct we might want to inspect its fields too if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && - subField->IsObjRef() && subField->GetFieldType() != ELEMENT_TYPE_VALUETYPE) + subField->IsObjRef()) { GCX_COOP(); From f8b8aabf5d32e3c9eb6b8f0910cd1b6c9e209dc4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 6 Mar 2023 21:36:27 +0100 Subject: [PATCH 12/17] Address feedback --- src/coreclr/jit/valuenum.cpp | 4 +- .../StaticReadonlyStructWithGC.cs | 47 +++++++++---------- .../StaticReadonlyStructWithGC.csproj | 1 - 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a59e480aad5f..b8ecd9c7af96 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5436,9 +5436,7 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel // if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && // it should be a static field - (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && - // boxed static to be precise - (fieldSeq->GetOffset() == TARGET_POINTER_SIZE)) + (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic)) { uint8_t buffer[TARGET_POINTER_SIZE] = {0}; if (((UINT)offset < INT_MAX) && diff --git a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs index 852906bbc3f2..b8227ece73c1 100644 --- a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs +++ b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.cs @@ -9,37 +9,32 @@ class StaticReadonlyStructWithGC { static int Main() { - for (int i = 0; i < 100; i++) - { - if (!Test1()) throw new Exception("Test1 failed"); - if (!Test2()) throw new Exception("Test2 failed"); - if (!Test3()) throw new Exception("Test3 failed"); - if (!Test4()) throw new Exception("Test4 failed"); - if (!Test5()) throw new Exception("Test5 failed"); - if (!Test6()) throw new Exception("Test6 failed"); - if (!Test7()) throw new Exception("Test7 failed"); - if (!Test8()) throw new Exception("Test8 failed"); - if (!Test9()) throw new Exception("Test9 failed"); + // Pre-initialize host type + RuntimeHelpers.RunClassConstructor(typeof(StaticReadonlyStructWithGC).TypeHandle); - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - Thread.Sleep(16); - } + if (!Test1()) throw new Exception("Test1 failed"); + if (!Test2()) throw new Exception("Test2 failed"); + if (!Test3()) throw new Exception("Test3 failed"); + if (!Test4()) throw new Exception("Test4 failed"); + if (!Test5()) throw new Exception("Test5 failed"); + if (!Test6()) throw new Exception("Test6 failed"); + if (!Test7()) throw new Exception("Test7 failed"); + if (!Test8()) throw new Exception("Test8 failed"); + if (!Test9()) throw new Exception("Test9 failed"); return 100; } static readonly MyStruct MyStructFld = new() - { - A = "A", - B = 111111.ToString(), // non-literal - C = new MyStruct2 { A = "AA" }, - D = typeof(int), - E = () => 42, - F = new MyStruct3 { A = typeof(double), B = typeof(string) }, - G = new int[0], - H = null - }; + { + A = "A", + B = 111111.ToString(), // non-literal + C = new MyStruct2 { A = "AA" }, + D = typeof(int), + E = () => 42, + F = new MyStruct3 { A = typeof(double), B = typeof(string) }, + G = new int[0], + H = null + }; [MethodImpl(MethodImplOptions.NoInlining)] static bool Test1() => MyStructFld.A == "A"; [MethodImpl(MethodImplOptions.NoInlining)] static bool Test2() => MyStructFld.B == "111111"; diff --git a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj index 9d5af3bce3d8..6946bed81bfd 100644 --- a/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj +++ b/src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj @@ -5,6 +5,5 @@ - From 7786e8e00754a014419e5f9f2e82f25e69829744 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 7 Mar 2023 21:56:30 +0100 Subject: [PATCH 13/17] Address feedback --- src/coreclr/jit/valuenum.cpp | 72 ++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index b8ecd9c7af96..c3527c589233 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -5426,41 +5426,6 @@ void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, Fiel { noway_assert(fieldSeq != nullptr); - // Check if the load represents a frozen gc object located in a struct which is stored in a static readonly field, - // e.g.: - // - // struct MyStruct { string Name; } - // static readonly MyStruct MyStr = new() { Name = "Hey!" }; - // - // string GetName() => MyStr.Name; // <- loadTree - // - if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && - // it should be a static field - (fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic)) - { - uint8_t buffer[TARGET_POINTER_SIZE] = {0}; - if (((UINT)offset < INT_MAX) && - info.compCompHnd->getReadonlyStaticFieldValue(fieldSeq->GetFieldHandle(), buffer, TARGET_POINTER_SIZE, - (int)offset)) - { - // In case of 64bit jit emitting 32bit codegen this handle will be 64bit - // value holding 32bit handle with upper half zeroed (hence, "= NULL"). - // It's done to match the current crossgen/ILC behavior. - ssize_t objHandle = 0; - memcpy(&objHandle, buffer, TARGET_POINTER_SIZE); - if (objHandle == 0) - { - loadTree->gtVNPair.SetBoth(vnStore->VNForNull()); - } - else - { - loadTree->gtVNPair.SetBoth(vnStore->VNForHandle(objHandle, GTF_ICON_OBJ_HDL)); - setMethodHasFrozenObjects(); - } - return; - } - } - // Two cases: // // 1) Instance field / "complex" static: heap[field][baseAddr][offset + load size]. @@ -10000,6 +9965,19 @@ void Compiler::fgValueNumberSsaVarDef(GenTreeLclVarCommon* lcl) // static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, ssize_t* byteOffset, FieldSeq** pFseq) { + VNFuncApp funcApp; + if (tree->TypeIs(TYP_REF) && vnStore->GetVNFunc(tree->gtVNPair.GetLiberal(), &funcApp) && + (funcApp.m_func == VNF_PtrToStatic)) + { + FieldSeq* fseq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]); + if (fseq->GetKind() == FieldSeq::FieldKind::SimpleStatic) + { + *byteOffset = vnStore->ConstantValue(funcApp.m_args[2]); + *pFseq = fseq; + return true; + } + } + ssize_t val = 0; // Special case for NativeAOT: ADD(ICON_STATIC, CNS_INT) where CNS_INT has field sequence corresponding to field's @@ -10083,7 +10061,7 @@ bool Compiler::fgValueNumberConstLoad(GenTreeIndir* tree) // ssize_t byteOffset = 0; FieldSeq* fieldSeq = nullptr; - if ((varTypeIsSIMD(tree) || varTypeIsIntegral(tree) || varTypeIsFloating(tree)) && + if ((varTypeIsSIMD(tree) || varTypeIsIntegral(tree) || varTypeIsFloating(tree) || tree->TypeIs(TYP_REF)) && GetStaticFieldSeqAndAddress(vnStore, tree->gtGetOp1(), &byteOffset, &fieldSeq)) { CORINFO_FIELD_HANDLE fieldHandle = fieldSeq->GetFieldHandle(); @@ -10162,6 +10140,20 @@ bool Compiler::fgValueNumberConstLoad(GenTreeIndir* tree) tree->gtVNPair.SetBoth(vnStore->VNForDoubleCon(val)); return true; } + case TYP_REF: + { + READ_VALUE(ssize_t); + if (val == 0) + { + tree->gtVNPair.SetBoth(vnStore->VNForNull()); + } + else + { + tree->gtVNPair.SetBoth(vnStore->VNForHandle(val, GTF_ICON_OBJ_HDL)); + setMethodHasFrozenObjects(); + } + return true; + } #if defined(FEATURE_SIMD) case TYP_SIMD8: { @@ -10549,6 +10541,10 @@ void Compiler::fgValueNumberTree(GenTree* tree) tree->gtVNPair = vnStore->VNPairForLoad(lclVNPair, lclSize, tree->TypeGet(), offset, loadSize); } + else if (tree->OperIs(GT_IND, GT_BLK, GT_OBJ) && fgValueNumberConstLoad(tree->AsIndir())) + { + // VN is assigned inside fgValueNumberConstLoad + } else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic)) { fldSeq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]); @@ -10557,10 +10553,6 @@ void Compiler::fgValueNumberTree(GenTree* tree) // Note VNF_PtrToStatic statics are currently always "simple". fgValueNumberFieldLoad(tree, /* baseAddr */ nullptr, fldSeq, offset); } - else if (tree->OperIs(GT_IND, GT_BLK, GT_OBJ) && fgValueNumberConstLoad(tree->AsIndir())) - { - // VN is assigned inside fgValueNumberConstLoad - } else if (vnStore->GetVNFunc(addrNvnp.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToArrElem)) { fgValueNumberArrayElemLoad(tree, &funcApp); From 552c4880b2a269c67fa2a8ac9715714c6acd40ab Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 8 Mar 2023 12:59:42 +0100 Subject: [PATCH 14/17] clean up --- src/coreclr/jit/valuenum.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c3527c589233..61d2da86b7cc 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -9966,8 +9966,7 @@ void Compiler::fgValueNumberSsaVarDef(GenTreeLclVarCommon* lcl) static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, ssize_t* byteOffset, FieldSeq** pFseq) { VNFuncApp funcApp; - if (tree->TypeIs(TYP_REF) && vnStore->GetVNFunc(tree->gtVNPair.GetLiberal(), &funcApp) && - (funcApp.m_func == VNF_PtrToStatic)) + if (vnStore->GetVNFunc(tree->gtVNPair.GetLiberal(), &funcApp) && (funcApp.m_func == VNF_PtrToStatic)) { FieldSeq* fseq = vnStore->FieldSeqVNToFieldSeq(funcApp.m_args[1]); if (fseq->GetKind() == FieldSeq::FieldKind::SimpleStatic) @@ -9977,7 +9976,6 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s return true; } } - ssize_t val = 0; // Special case for NativeAOT: ADD(ICON_STATIC, CNS_INT) where CNS_INT has field sequence corresponding to field's From 3a63145c4961521ecea5bdde869447e364e628b0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 9 Mar 2023 19:06:50 +0100 Subject: [PATCH 15/17] Update src/coreclr/vm/jitinterface.cpp Co-authored-by: Jan Kotas --- src/coreclr/vm/jitinterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index f9ada5b2fe84..df2fa47b70ac 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11699,7 +11699,7 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t for (FieldDesc* subField = fieldIterator.Next(); subField != NULL; subField = fieldIterator.Next()) { // TODO: If subField is also a struct we might want to inspect its fields too - if (subField->GetOffset() == (DWORD)valueOffset && subField->GetSize() == (UINT)bufferSize && + if (subField->GetOffset() == (DWORD)valueOffset && sizeof(CORINFO_OBJECT_HANDLE)) == (UINT)bufferSize && subField->IsObjRef()) { GCX_COOP(); From 6dc936f4ed77ed1122ce59c76247127b2d4057a5 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 9 Mar 2023 19:18:51 +0100 Subject: [PATCH 16/17] hoist the check --- src/coreclr/vm/jitinterface.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index df2fa47b70ac..5e30e9f61654 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11693,14 +11693,13 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t if (!field->IsRVA() && field->GetFieldType() == ELEMENT_TYPE_VALUETYPE) { PTR_MethodTable structType = field->GetFieldTypeHandleThrowing().AsMethodTable(); - if (structType->ContainsPointers()) + if (structType->ContainsPointers() && (UINT)bufferSize == sizeof(CORINFO_OBJECT_HANDLE)) { ApproxFieldDescIterator fieldIterator(structType, ApproxFieldDescIterator::INSTANCE_FIELDS); for (FieldDesc* subField = fieldIterator.Next(); subField != NULL; subField = fieldIterator.Next()) { // TODO: If subField is also a struct we might want to inspect its fields too - if (subField->GetOffset() == (DWORD)valueOffset && sizeof(CORINFO_OBJECT_HANDLE)) == (UINT)bufferSize && - subField->IsObjRef()) + if (subField->GetOffset() == (DWORD)valueOffset && subField->IsObjRef()) { GCX_COOP(); From fe4543a44c77e930ead8500b255e21e57d13b2c3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 9 Mar 2023 19:27:17 +0100 Subject: [PATCH 17/17] Address feedback --- src/coreclr/vm/jitinterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 5e30e9f61654..38939b28ee5d 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -11728,7 +11728,7 @@ bool CEEInfo::getReadonlyStaticFieldValue(CORINFO_FIELD_HANDLE fieldHnd, uint8_t } } } - else + else if (!structType->ContainsPointers()) { // No gc pointers in the struct memcpy(buffer, (uint8_t*)baseAddr + valueOffset, bufferSize);