Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
da7b00b
initial port from the prototype
VSadov Feb 25, 2023
170a306
parse the attribute in MT builder
VSadov Feb 26, 2023
bb1c3db
validate replicated size
VSadov Feb 26, 2023
0a4cd78
aot changes
VSadov Feb 26, 2023
6eeb887
refmap
VSadov Feb 27, 2023
f0d156b
validate total size in ilc
VSadov Feb 27, 2023
470aee2
add the actual attribute
VSadov Feb 28, 2023
78ab523
Apply suggestions from code review
VSadov Feb 28, 2023
a452be7
add a small use of InlineArray in CoreLib - to get some crossgen cove…
VSadov Feb 28, 2023
2801f50
a few more uses in CorLib
VSadov Mar 1, 2023
e3284d1
fix for sequential layout
VSadov Mar 1, 2023
4ff0a25
Standardize on use of "InlineArray" in the implementation
VSadov Mar 2, 2023
b406dfe
simpler layout replication
VSadov Mar 2, 2023
7f197d6
some initial tests (will add more)
VSadov Mar 3, 2023
1852993
more tests
VSadov Mar 4, 2023
295caf1
limit the max size of array instance to 1MiB
VSadov Mar 4, 2023
e7738ea
fix an assert in importercalls.cpp
VSadov Mar 4, 2023
065467b
"result" in GC layout should track the pointer count, not the size
VSadov Mar 4, 2023
62b20c8
error messages
VSadov Mar 8, 2023
01e2e17
fixed uses of "value array" in comments.
VSadov Mar 8, 2023
f844772
PR feedback on StackAllocedArguments
VSadov Mar 8, 2023
1b5f608
use the same size limit for inline arrays in the typeloader
VSadov Mar 8, 2023
bf676e4
more PR feedback
VSadov Mar 8, 2023
7d42fbc
remove SetCannotBeBlittedByObjectCloner
VSadov Mar 8, 2023
7fff2be
moving GetInlineArrayLength to MetadataType and related changes
VSadov Mar 10, 2023
2d403ee
use type.Context.Target.PointerSize
VSadov Mar 10, 2023
f6f6acc
lost resources change
VSadov Mar 10, 2023
d68cfa2
fix for x86
VSadov Mar 12, 2023
ebc698b
Do not make InlineArrayType an inline array just yet.
VSadov Mar 13, 2023
9373c77
CORINFO_FLG_INDEXABLE_FIELDS
VSadov Mar 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
more PR feedback
  • Loading branch information
VSadov committed Mar 10, 2023
commit bf676e4d19644134ef73bbece091740799438747
4 changes: 2 additions & 2 deletions src/coreclr/vm/class.h
Original file line number Diff line number Diff line change
Expand Up @@ -1384,12 +1384,12 @@ class EEClass // DO NOT CREATE A NEW EEClass USING NEW!
LIMITED_METHOD_CONTRACT;
m_VMFlags |= (DWORD)VMFLAG_HAS_FIELDS_WHICH_MUST_BE_INITED;
}
BOOL HasInlineArrayFlagSet()
DWORD IsInlineArray()
{
LIMITED_METHOD_CONTRACT;
return (m_VMFlags & VMFLAG_INLINE_ARRAY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return (m_VMFlags & VMFLAG_INLINE_ARRAY);
return (m_VMFlags & VMFLAG_INLINE_ARRAY) ? TRUE : FALSE;

Or please change the BOOL to a DWORD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively just use bool and update to ? true : false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of the helpers in this file do ? TRUE : FALSE; , but some return DWORD.
I'll make this DWORD, to fit the style.

}
void SetInlineArrayFlag()
void SetIsInlineArray()
{
LIMITED_METHOD_CONTRACT;
m_VMFlags |= (DWORD)VMFLAG_INLINE_ARRAY;
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2169,7 +2169,7 @@ static unsigned ComputeGCLayout(MethodTable* pMT, BYTE* gcPtrs)
_ASSERTE(pMT->IsValueType());

unsigned result = 0;
bool isInlineArray = pMT->GetClass()->HasInlineArrayFlagSet();
bool isInlineArray = pMT->GetClass()->IsInlineArray();
ApproxFieldDescIterator fieldIterator(pMT, ApproxFieldDescIterator::INSTANCE_FIELDS);
for (FieldDesc *pFD = fieldIterator.Next(); pFD != NULL; pFD = fieldIterator.Next())
{
Expand Down Expand Up @@ -3569,7 +3569,7 @@ uint32_t CEEInfo::getClassAttribsInternal (CORINFO_CLASS_HANDLE clsHnd)
if (pClass->HasExplicitFieldOffsetLayout() && pClass->HasOverlaidField())
ret |= CORINFO_FLG_OVERLAPPING_FIELDS;

if (pClass->HasInlineArrayFlagSet())
if (pClass->IsInlineArray())
ret |= CORINFO_FLG_DONT_DIG_FIELDS;

if (VMClsHnd.IsCanonicalSubtype())
Expand Down
8 changes: 5 additions & 3 deletions src/coreclr/vm/methodtablebuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1715,7 +1715,7 @@ MethodTableBuilder::BuildMethodTableThrowing(
if (repeat > 0)
{
bmtFP->NumInlineArrayElements = repeat;
GetHalfBakedClass()->SetInlineArrayFlag();
GetHalfBakedClass()->SetIsInlineArray();
}
else
{
Expand Down Expand Up @@ -1749,7 +1749,7 @@ MethodTableBuilder::BuildMethodTableThrowing(

_ASSERTE(HasLayout());

if (bmtFP->NumInlineArrayElements)
if (bmtFP->NumInlineArrayElements != 0)
{
GetLayoutInfo()->m_cbManagedSize *= bmtFP->NumInlineArrayElements;
}
Expand Down Expand Up @@ -11556,7 +11556,6 @@ VOID MethodTableBuilder::HandleGCForValueClasses(MethodTable ** pByValueClassCac
DWORD repeat = 1;
if (bmtFP->NumInlineArrayElements > 1)
{
_ASSERTE(bmtEnumFields->dwNumInstanceFields == 1);
repeat = bmtFP->NumInlineArrayElements;
}

Expand All @@ -11583,6 +11582,9 @@ VOID MethodTableBuilder::HandleGCForValueClasses(MethodTable ** pByValueClassCac
DWORD dwCurrentOffset = pFieldDescList[i].GetOffset_NoLogging();
DWORD dwElementSize = pByValueMT->GetBaseSize() - OBJECT_BASESIZE;

// if we have an inline array, we will have only one formal instance field,
// but will have to replicate the layout "repeat" times.
// otherwise every field will be matched with 1 serie.
for (DWORD r = 0; r < repeat; r++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the value is fully covered by one series, we should only emit one series for the repeated value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in arrays? I think that pattern requires that the element count is stored in the instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for side-by-side objects. That we can express as one combined sequence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems like can be handled in more general terms - including regular structs.
If it is an all-objects instance (recursively) and it is not an explicit layout, we can put in just one serie for the entire size of the instance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at this from implementation point. Folding GC series for side-by-side objects from separate struct fields is certainly doable, but a bit involved, especially on the CoreClr side.

I think it is better if this is handled as a separate PR as it is an optimization and we may need to iterate a bit on it. There is also some risk there will be bugs or regressions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged an issue for this - #83111

{
// The by value class may have more than one pointer series
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/vm/siginfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4862,11 +4862,11 @@ class ByRefPointerOffsetsReporter
_ASSERTE(pMT != nullptr);
_ASSERTE(pMT->IsByRefLike());

bool isValArray = pMT->GetClass()->HasInlineArrayFlagSet();
bool isValArray = pMT->GetClass()->IsInlineArray();
ApproxFieldDescIterator fieldIterator(pMT, ApproxFieldDescIterator::INSTANCE_FIELDS);
for (FieldDesc* pFD = fieldIterator.Next(); pFD != NULL; pFD = fieldIterator.Next())
{
if(isValArray)
if (isValArray)
{
_ASSERTE(pFD->GetOffset() == 0);
DWORD elementSize = pFD->GetSize();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

namespace System.Runtime.CompilerServices
{
/// <summary>
/// Indicates that the instance's storage is sequentially replicated "length" times.
/// </summary>
[EditorBrowsable(EditorBrowsableState.Never)]
[AttributeUsage(AttributeTargets.Struct, AllowMultiple = false)]
public sealed class InlineArrayAttribute : Attribute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add /// comments now so the doc will properly flow.

/cc @carlossanlop

Expand Down
23 changes: 23 additions & 0 deletions src/tests/Loader/classloader/InlineArray/InlineArrayInvalid.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,29 @@ public static void TooLarge_Fails()
});
}

[InlineArray(-1)]
private struct NegativeLength
{
public long field;
}

[Fact]
public static void NegativeLength_Fails()
{
Console.WriteLine($"{nameof(NegativeLength_Fails)}...");
Assert.Throws<TypeLoadException>(() => { var t = typeof(NegativeLength); });

Assert.Throws<TypeLoadException>(() =>
{
var t = new NegativeLength()
{
field = 1
};
return t;
});
}


[InlineArray(123)]
private struct NoFields
{
Expand Down
2 changes: 1 addition & 1 deletion src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -1530,7 +1530,7 @@
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/Loader/classloader/InlineArray/**">
<Issue>InlineArray support is not yet implemented in the Mono runtime.</Issue>
<Issue>https://github.com/dotnet/runtime/issues/80798</Issue>
</ExcludeList>

<ExcludeList Include="$(XunitTestBinBase)/baseservices/exceptions/stackoverflow/stackoverflowtester/**">
Expand Down