Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[x64][SysV] Classify empty structs for passing like padding #103799

Merged
merged 4 commits into from
Jun 28, 2024
Merged

[x64][SysV] Classify empty structs for passing like padding #103799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aba87b6
Classify empty structs on x64 like padding
tomeksowi May 8, 2024
131a3c5
Add comment to fix JIT to AssignClassifiedEightByteTypes
tomeksowi Jun 21, 2024
cb22839
Add tests for empty struct field passing on System V
tomeksowi Jun 24, 2024
db406a7
Update field names in C++ tests to match C#
tomeksowi Jun 24, 2024
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
23 changes: 18 additions & 5 deletions src/coreclr/tools/Common/JitInterface/SystemVStructClassificator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using ILCompiler;
using Internal.TypeSystem;
using System.Runtime.CompilerServices;
using static Internal.JitInterface.SYSTEMV_AMD64_CORINFO_STRUCT_REG_PASSING_DESCRIPTOR;
using static Internal.JitInterface.SystemVClassificationType;

Expand Down Expand Up @@ -207,7 +208,10 @@ private static bool ClassifyEightBytes(TypeDesc typeDesc,

if (numIntroducedFields == 0)
{
return false;
// Classify empty struct like padding
helper.LargestFieldOffset = startOffsetOfStruct;
AssignClassifiedEightByteTypes(ref helper);
return true;
}

// The SIMD and Int128 Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -375,8 +379,12 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
int offsetAfterLastFieldByte = largestFieldOffset + helper.FieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helper.FieldClassifications[lastFieldOrdinal];
int lastFieldSize = (lastFieldOrdinal >= 0) ? helper.FieldSizes[lastFieldOrdinal] : 0;
int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
Debug.Assert(offsetAfterLastFieldByte <= helper.StructSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helper.FieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

int usedEightBytes = 0;
int accumulatedSizeForEightBytes = 0;
Expand All @@ -403,6 +411,8 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -455,13 +465,16 @@ private static void AssignClassifiedEightByteTypes(ref SystemVStructRegisterPass
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helper.StructSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
Copy link
Member

Choose a reason for hiding this comment

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

What does the difference between NoClass and something like char[8] end up being? For significant padding (structs with explicit layout) it seems like we have to consider them to be the same or things will end up odd/wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

char[8] is an INTEGER eightbyte which gets assigned an integer register for passing; a NO_CLASS eightbyte doesn't get any register. Passing on the stack is the same, though (NO_CLASS padding does take up space).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #104098 as I'll need an URL for ActiveIssue in tests in future PRs.

Meanwhile this smaller fix is ready for review.

Copy link
Member

Choose a reason for hiding this comment

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

Does this fix handle the significant padding cases correctly (i.e. as before this change)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, the NO_CLASS -> INTEGER eightbyte reclassification is still there. And I didn't see any CLR tests (prio1) go off on Checked build, assuming there are tests for significant padding by now.

if (helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helper.EightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down
35 changes: 26 additions & 9 deletions src/coreclr/vm/methodtable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2114,11 +2114,14 @@ bool MethodTable::ClassifyEightBytesWithManagedLayout(SystemVStructRegisterPassi

DWORD numIntroducedFields = GetNumIntroducedInstanceFields();

// It appears the VM gives a struct with no fields of size 1.
// Don't pass in register such structure.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

// The SIMD Intrinsic types are meant to be handled specially and should not be passed as struct registers
Expand Down Expand Up @@ -2334,7 +2337,12 @@ bool MethodTable::ClassifyEightBytesWithNativeLayout(SystemVStructRegisterPassin
// No fields.
if (numIntroducedFields == 0)
{
return false;
helperPtr->largestFieldOffset = startOffsetOfStruct;
LOG((LF_JIT, LL_EVERYTHING, "%*s**** Classify empty struct %s (%p) like padding, startOffset %d, total struct size %d\n",
nestingLevel * 5, "", this->GetDebugClassName(), this, startOffsetOfStruct, helperPtr->structSize));

AssignClassifiedEightByteTypes(helperPtr, nestingLevel);
return true;
}

bool hasImpliedRepeatedFields = HasImpliedRepeatedFields(this);
Expand Down Expand Up @@ -2586,8 +2594,12 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// Calculate the eightbytes and their types.

int lastFieldOrdinal = sortedFieldOrder[largestFieldOffset];
unsigned int offsetAfterLastFieldByte = largestFieldOffset + helperPtr->fieldSizes[lastFieldOrdinal];
SystemVClassificationType lastFieldClassification = helperPtr->fieldClassifications[lastFieldOrdinal];
unsigned int lastFieldSize = (lastFieldOrdinal >= 0) ? helperPtr->fieldSizes[lastFieldOrdinal] : 0;
unsigned int offsetAfterLastFieldByte = largestFieldOffset + lastFieldSize;
_ASSERTE(offsetAfterLastFieldByte <= helperPtr->structSize);
SystemVClassificationType lastFieldClassification = (lastFieldOrdinal >= 0)
? helperPtr->fieldClassifications[lastFieldOrdinal]
: SystemVClassificationTypeNoClass;

unsigned int usedEightBytes = 0;
unsigned int accumulatedSizeForEightBytes = 0;
Expand All @@ -2614,6 +2626,8 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
// the SysV ABI spec.
fieldSize = 1;
fieldClassificationType = offset < offsetAfterLastFieldByte ? SystemVClassificationTypeNoClass : lastFieldClassification;
if (offset % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // new eightbyte
foundFieldInEightByte = false;
}
else
{
Expand Down Expand Up @@ -2666,13 +2680,16 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
}
}

if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0) // If we just finished checking the last byte of an eightbyte
// If we just finished checking the last byte of an eightbyte or the entire struct
if ((offset + 1) % SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES == 0 || (offset + 1) == helperPtr->structSize)
{
if (!foundFieldInEightByte)
{
// If we didn't find a field in an eight-byte (i.e. there are no explicit offsets that start a field in this eightbyte)
// If we didn't find a field in an eightbyte (i.e. there are no explicit offsets that start a field in this eightbyte)
// then the classification of this eightbyte might be NoClass. We can't hand a classification of NoClass to the JIT
// so set the class to Integer (as though the struct has a char[8] padding) if the class is NoClass.
//
// TODO: Fix JIT, NoClass eightbytes are valid and passing them is broken because of this.
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to fix this TODO in this PR?

Copy link
Contributor Author

@tomeksowi tomeksowi Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No. It would require going through the JIT like I did in #101796 for RISC-V and fix in many places. Right now my priority is on RISC-V but when I'm done with #101796 I can revisit it. By then @jakobbotsch's ongoing effort to centralize ABI passing infos will probably be more advanced, which will also facilitate fixing x64.

if (helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] == SystemVClassificationTypeNoClass)
{
helperPtr->eightByteClassifications[offset / SYSTEMV_EIGHT_BYTE_SIZE_IN_BYTES] = SystemVClassificationTypeInteger;
Expand Down Expand Up @@ -2705,9 +2722,9 @@ void MethodTable::AssignClassifiedEightByteTypes(SystemVStructRegisterPassingHe
LOG((LF_JIT, LL_EVERYTHING, " **** Number EightBytes: %d\n", helperPtr->eightByteCount));
for (unsigned i = 0; i < helperPtr->eightByteCount; i++)
{
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
LOG((LF_JIT, LL_EVERYTHING, " **** eightByte %d -- classType: %s, eightByteOffset: %d, eightByteSize: %d\n",
i, GetSystemVClassificationTypeName(helperPtr->eightByteClassifications[i]), helperPtr->eightByteOffsets[i], helperPtr->eightByteSizes[i]));
_ASSERTE(helperPtr->eightByteClassifications[i] != SystemVClassificationTypeNoClass);
}
#endif // _DEBUG
}
Expand Down
6 changes: 4 additions & 2 deletions src/tests/JIT/Directed/StructABI/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ project (StructABILib)
include_directories(${INC_PLATFORM_DIR})

if(CLR_CMAKE_HOST_WIN32)
add_compile_options(/TC) # compile all files as C
set_source_files_properties(StructABI.c PROPERTIES COMPILE_OPTIONS /TC) # compile as C
else()
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fvisibility=hidden")
set(CMAKE_CPP_FLAGS "${CMAKE_CPP_FLAGS} -fvisibility=hidden -Wno-return-type-c-linkage")
endif()

# add the executable
add_library (StructABILib SHARED StructABI.c)
add_library (EmptyStructsLib SHARED EmptyStructs.cpp)

# add the install targets
install (TARGETS StructABILib DESTINATION bin)
install (TARGETS StructABILib EmptyStructsLib DESTINATION bin)
68 changes: 68 additions & 0 deletions src/tests/JIT/Directed/StructABI/EmptyStructs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

#include <stdint.h>
#include <stddef.h>

#ifdef _MSC_VER
#define DLLEXPORT __declspec(dllexport)
#else
#define DLLEXPORT __attribute__((visibility("default")))
#endif // _MSC_VER

struct Empty
{
};
static_assert(sizeof(Empty) == 1, "Empty struct must be sized like in .NET");


struct IntEmpty
{
int32_t Int0;
Empty Empty0;
};

extern "C" DLLEXPORT IntEmpty EchoIntEmptySysV(int i0, IntEmpty val)
{
return val;
}


struct IntEmptyPair
{
IntEmpty IntEmpty0;
IntEmpty IntEmpty1;
};

extern "C" DLLEXPORT IntEmptyPair EchoIntEmptyPairSysV(int i0, IntEmptyPair val)
{
return val;
}


struct EmptyFloatIntInt
{
Empty Empty0;
float Float0;
int32_t Int0;
int32_t Int1;
};

extern "C" DLLEXPORT EmptyFloatIntInt EchoEmptyFloatIntIntSysV(int i0, float f0, EmptyFloatIntInt val)
{
return val;
}


struct FloatFloatEmptyFloat
{
float Float0;
float Float1;
Empty Empty0;
float Float2;
};

extern "C" DLLEXPORT FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysV(float f0, FloatFloatEmptyFloat val)
{
return val;
}

150 changes: 150 additions & 0 deletions src/tests/JIT/Directed/StructABI/EmptyStructs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
// 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.InteropServices;
using System.Runtime.CompilerServices;
using Xunit;

public static class Program
{
public struct Empty
{
}


public struct IntEmpty
{
public int Int0;
public Empty Empty0;

public static IntEmpty Get()
=> new IntEmpty { Int0 = 0xBabc1a };

public bool Equals(IntEmpty other)
=> Int0 == other.Int0;

public override string ToString()
=> $"{{Int0:{Int0:x}}}";
}

[DllImport("EmptyStructsLib")]
public static extern IntEmpty EchoIntEmptySysV(int i0, IntEmpty val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static IntEmpty EchoIntEmptySysVManaged(int i0, IntEmpty val) => val;

[Fact]
public static void TestIntEmptySysV()
{
IntEmpty expected = IntEmpty.Get();
IntEmpty native = EchoIntEmptySysV(0, expected);
IntEmpty managed = EchoIntEmptySysVManaged(0, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct IntEmptyPair
{
public IntEmpty IntEmpty0;
public IntEmpty IntEmpty1;

public static IntEmptyPair Get()
=> new IntEmptyPair { IntEmpty0 = IntEmpty.Get(), IntEmpty1 = IntEmpty.Get() };

public bool Equals(IntEmptyPair other)
=> IntEmpty0.Equals(other.IntEmpty0) && IntEmpty1.Equals(other.IntEmpty1);

public override string ToString()
=> $"{{IntEmpty0:{IntEmpty0}, IntEmpty1:{IntEmpty1}}}";
}

[DllImport("EmptyStructsLib")]
public static extern IntEmptyPair EchoIntEmptyPairSysV(int i0, IntEmptyPair val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static IntEmptyPair EchoIntEmptyPairSysVManaged(int i0, IntEmptyPair val) => val;

[Fact]
public static void TestIntEmptyPairSysV()
{
IntEmptyPair expected = IntEmptyPair.Get();
IntEmptyPair native = EchoIntEmptyPairSysV(0, expected);
IntEmptyPair managed = EchoIntEmptyPairSysVManaged(0, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct EmptyFloatIntInt
{
public Empty Empty0;
public float Float0;
public int Int0;
public int Int1;

public static EmptyFloatIntInt Get()
=> new EmptyFloatIntInt { Float0 = 2.71828f, Int0 = 0xBabc1a, Int1 = 0xC10c1a };

public bool Equals(EmptyFloatIntInt other)
=> Float0 == other.Float0 && Int0 == other.Int0 && Int1 == other.Int1;

public override string ToString()
=> $"{{Float0:{Float0}, Int0:{Int0:x}, Int1:{Int1:x}}}";
}

[DllImport("EmptyStructsLib")]
public static extern EmptyFloatIntInt EchoEmptyFloatIntIntSysV(int i0, float f0, EmptyFloatIntInt val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static EmptyFloatIntInt EchoEmptyFloatIntIntSysVManaged(int i0, float f0, EmptyFloatIntInt val) => val;

[Fact]
public static void TestEmptyFloatIntIntSysV()
{
EmptyFloatIntInt expected = EmptyFloatIntInt.Get();
EmptyFloatIntInt native = EchoEmptyFloatIntIntSysV(0, 0f, expected);
EmptyFloatIntInt managed = EchoEmptyFloatIntIntSysVManaged(0, 0f, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}


public struct FloatFloatEmptyFloat
{
public float Float0;
public float Float1;
public Empty Empty0;
public float Float2;

public static FloatFloatEmptyFloat Get()
=> new FloatFloatEmptyFloat { Float0 = 2.71828f, Float1 = 3.14159f, Float2 = 1.61803f };

public bool Equals(FloatFloatEmptyFloat other)
=> Float0 == other.Float0 && Float1 == other.Float1 && Float2 == other.Float2;

public override string ToString()
=> $"{{Float0:{Float0}, Float1:{Float1}, Float2:{Float2}}}";
}

[DllImport("EmptyStructsLib")]
public static extern FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysV(float f0, FloatFloatEmptyFloat val);

[MethodImpl(MethodImplOptions.NoInlining)]
public static FloatFloatEmptyFloat EchoFloatFloatEmptyFloatSysVManaged(float f0, FloatFloatEmptyFloat val) => val;

[Fact]
public static void TestFloatFloatEmptyFloatSysV()
{
FloatFloatEmptyFloat expected = FloatFloatEmptyFloat.Get();
FloatFloatEmptyFloat native = EchoFloatFloatEmptyFloatSysV(0f, expected);
FloatFloatEmptyFloat managed = EchoFloatFloatEmptyFloatSysVManaged(0f, expected);

Assert.Equal(expected, native);
Assert.Equal(expected, managed);
}
}
Loading
Loading