[RISC-V][x64] WiP: Passing empty struct fields

[tomeksowi]

This PR should be viewed as proof of concept, it will be upstreamed in smaller chunks so it can be reviewed with some confidence.

RISC-V and LoongArch

Small structs containing one or two fields, at least one of them floating-point, can be passed (or returned) according to hardware FP calling convention: each field occupying one register. The existing implementation worked only for the narrow case when the fields were naturally aligned. However, the ABIs on both platforms when enregistering fields disregard placement hints such as manual alignment, packing attributes, or padding with empty structs (when they are sized 1 byte like in C++ or .NET). This means additional information on field offsets and sizes needs to be passed wherever registers<->memory copying of such structs happens.

RISC-V only: Unlike LoongArch's, RISC-V's ABI does not bound the size of such structs to 16 bytes. This means, among other things, that we can no longer rule out struct's eligibility for passing according to hardware FP calling convention by simply checking size > 16, which is assumed in many places.

System V x86-64

The current implementation barred a struct containing empty struct fields from enregistration. This did not match the System V ABI which says "NO_CLASS This class is used as initializer in the algorithms. It will be used for padding and empty structures and unions". It also does not match the behavior of GCC & Clang on Linux.

Part of #84834, cc @dotnet/samsung

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[tomeksowi]

For the record, I don't think Pack should be necessary. From what I dug in the codebase, it's a known problem:

runtime/src/coreclr/vm/jitinterface.cpp

Lines 2102 to 2112 in ce2364c

	if (result < 8 && pMT->RequiresAlign8())
	{
	// If the structure contains 64-bit primitive fields and the platform requires 8-byte alignment for
	// such fields then make sure we return at least 8-byte alignment. Note that it's technically possible
	// to create unmanaged APIs that take unaligned structures containing such fields and this
	// unconditional alignment bump would cause us to get the calling convention wrong on platforms such
	// as ARM. If we see such cases in the future we'd need to add another control (such as an alignment
	// property for the StructLayout attribute or a marshaling directive attribute for p/invoke arguments)
	// that allows more precise control. For now we'll go with the likely scenario.
	result = 8;
	}

IMHO the fix would be something like:

• In CalculateSizeAndFieldOffsets amend calculating alignmentRequirement to:
  GCD(min(alignmentRequirement, packingSize), placementInfo->m_offset)
• Amend this condition to take explicit offset into consideration:
  

  runtime/src/coreclr/vm/methodtablebuilder.cpp

  Lines 4532 to 4540 in ce2364c

  	// For types with layout we drop any 64-bit alignment requirement if the packing size was less than 8
  	// bytes (this mimics what the native compiler does and ensures we match up calling conventions during
  	// interop).
  	// We don't do this for types that are marked as sequential but end up with auto-layout due to containing pointers,
  	// as auto-layout ignores any Pack directives.
  	if (HasLayout() && (HasExplicitFieldOffsetLayout() || IsManagedSequential()) && GetLayoutInfo()->GetPackingSize() < 8)
  	{
  	fFieldRequiresAlign8 = false;
  	}

But since this doesn't have much to do with empty structs, I'll leave it for now to keep this PR focused.

[jakobbotsch]

Can you share some information about the case this fixes? Given that promoted struct fields are normalize-on-load, this special case should not be necessary unless there is a bug elsewhere in the RISCV64/LA64 backends.

[tomeksowi]

It was for cases like this:

runtime/src/tests/JIT/Directed/StructABI/StructABI.cs

Lines 691 to 707 in 7551d35

	public struct EmptyFloatEmpty5Byte
	{
	public Empty e;
	public float FieldF;
	public Empty e0, e1, e2, e3, e4;
	public sbyte FieldB;
	public static EmptyFloatEmpty5Byte Get()
	{
	return new EmptyFloatEmpty5Byte { FieldF = 3.14159f, FieldB = -123 };
	}
	public bool Equals(EmptyFloatEmpty5Byte other)
	{
	return FieldF.Equals(other.FieldF) && FieldB == other.FieldB;
	}
	}

Homing the argument in the EmptyFloatEmpty5Byte:Equals prolog trashed the this pointer:

IN0016: 000000      addi           sp, sp, -48
IN0017: 000004      sd             fp, 32(sp)
IN0018: 000008      sd             ra, 40(sp)
IN0019: 00000C      addi           fp, sp, 32
IN001a: 000010      sd             a0, -8(fp)  // store 'this'
IN001b: 000014      fsw            f10, -20(fp)
IN001c: 000018      sw             a1, -11(fp)  // stomp on 'this'

I saw native compilers home arguments with appropriately-sized stores at original struct offsets so I fixed it the same way.

Full JitDump of EmptyFloatEmpty5Byte:Equals if interested

[jakobbotsch]

Got it, I think the fix makes sense. The comment doesn't seem fully accurate then; the real problem seems to be that since the enregistered layout does not match the memory layout of the struct, rounding up the size would potentially extend outside the stack slot.

[tomeksowi]

Right, I'll reword the comment.

Oh, and please view this PR as a proof of concept. I need to upstream it in smaller chunks so it can be reviewed with some confidence.

[jakobbotsch]

As I alluded to in some other PRs we have the notion of significant padding where it can essentially be considered that all the padding of a struct is covered by fields. If you look at the getTypeLayout implementation you can see how it is computed. The JIT takes care to preserve values in bytes not covered by fields in those cases, and the ABI classification will need to do the same.

For example, a structure declaration like

private unsafe struct S
{
    public fixed byte Foo[15];
    public float Bar;
}

results in underlying metadata that looks like

private unsafe struct S
{
  public FooStruct Foo;
  public float Bar;
}

[StructLayout(LayoutKind.Sequential, Size = 15)]
private struct FooStruct
{
  public byte FixedElementField;
}

I'm curious how this code ends up classifying a struct like this one for passing.

It seems like RISC-V/LA64 are going to end up with some potentially surprising user behavior here because of these ABI differences when padding is involved, and because it is somewhat ambiguous to the VM/JIT what is (ignorable) padding (or at least not totally obvious to the user).

[tomeksowi]

Here's a log from a similar case in JIT/Directed/StructABI/StructABI/StructABI.cs:

TID 3f2277: FpStructInRegistersInfo: flattening InlineArray1 (managed, 1 fields)
TID 3f2277: FpStructInRegistersInfo:     flattening <Array>e__FixedBuffer (managed, 1 fields)
TID 3f2277: FpStructInRegistersInfo:      * found field FixedElementField [0..1), type: Byte
TID 3f2277: FpStructInRegistersInfo:          * array has too many elements: 16

So it stops the field flattening because there's too many fields (max 2 fields to get passed according to FP calling convention) and returns an empty FpStructInRegistersInfo which means pass according to integer calling convention where there is no notion of "fields", structs are a lump of bits laid out in registers as in memory. But it this case struct S is bigger than 16 bytes so it's passed by implicit ref.

[jakobbotsch]

I see now the handling for HasImpliedRepeatedField; that might have to be generalized somewhat. What about the following example?

[StructLayout(LayoutKind.Explicit, Size = 20)]
struct S
{
  [FieldOffset(0)]
  public byte FirstByteOfArray;
  [FieldOffset(16)]
  public float FloatField;
}

Maybe SysV classification needs some generalization too (I can see it uses HasImpliedRepeatedFields too). Or perhaps it is the significant padding computation in getTypeLayout that is overly conservative.

[tomeksowi]

I think as expected:

TID 3f849f: FpStructInRegistersInfo: flattening S (managed, 2 fields)
TID 3f849f: FpStructInRegistersInfo:  * found field FirstByteOfArray [0..1), type: Byte
TID 3f849f: FpStructInRegistersInfo:  * found field FloatField [12..16), type: Single
TID 3f849f: FpStructInRegistersInfo: struct S (16 bytes) can be passed with floating-point calling convention, flags=0x88; IntFloat, sizes={1, 4}, offsets={0, 12}, IntFieldKindMask=Integer

Note: I downsized to 16 bytes as I'm on #103945 branch where the condition has not been relaxed for RISC-V. But I think it answers your question (padding via explicit layout).

There are problems with handling fixed buffers as the comment in HasImpliedRepeatedField implies. But as far as the classification for RISC-V goes I think it's ok.

[jakobbotsch]

getTypeLayout considers the above to be equivalent to a struct struct S {public fixed byte Array[16]; public float FloatField; }. So in that view I don't think the RISC-V classification is ok; IIUC it will silently drop parts of the struct that may contain user data when it gets passed as an argument.

The ABI classification here should match what getTypeLayout decides, one way or the other. I'm not sure if it is possible for us to change what it considers significant padding, since that has been in the JIT for a long time now.

[jakobbotsch]

How would the user write data to e.g. FirstByteOfArray[5] with standard language features?

Using unsafe code. I am not sure of the historical details of how things evolved this way, but I would guess C++/CLI is a large part of it.

Native compilers in general don't consider padding to be preserved while passing, definitely not the RISC-V ABI. That's the whole point of passing a struct according to FP calling convention, as two fields each in one register.

The difference in .NET is simply what we consider to be the discardable padding. We also have discardable padding, like the last 4 bytes of Span<T>. But ExplicitLayout/explicit size automatically promote the padding of the struct to be considered as "must be preserved".
I believe most of your example test cases have discardable padding since they do not have ExplicitLayout/explicit size, so there I think the ABI classification done here makes sense. But for the example given above things are different, where the ABI classification should essentially be done as if the padding was replaced by explicit char arrays of the right size.

BTW what would be the .NET equivalent for this C/C++ struct?

struct {
    char i;
    alignas(16) float f;
};

I am not sure we have an equivalent. I think there are both .NET structs (like the ones with ExplicitLayout/Size) that are not representable in C/C++; and C/C++ structs (like your example) that are not representable in .NET metadata. @AaronRobinsonMSFT, @jkoritzinsky or @jkotas should know more about the interop story here...

That's news to me:/ I don't see getTypeLayout used for parameter classification neither on System V nor on RISC-V/LoongArch.

I mention getTypeLayout because it has the current source of truth of when we consider padding in structs to be significant/required to be preserved. I do not know if the rules in there could be relaxed; just that changing the rules would be breaking, and that users are potentially relying on the values in padding of such structures to be preserved. So in that sense it becomes a problem if the ABI does not match with these rules.

I believe all of our existing ABIs come with rules that will preserve this padding, and thus we have not needed to pay any special attention to it before. I could be wrong about this on SysV, in which case I would consider it a bug.

Ultimately I think getTypeLayout and the ABI classification need to agree on what padding in a struct is significant and needs to be preserved, but I do not know whether it would be possible to relax the rules in getTypeLayout or not.

[jakobbotsch]

Are there any rules defined for what padding is considered significant or just whatever is in the code for getTypeLayout? Becuase treating padding as an array does influence the classification for passing so the user would need to be aware of it. The remarks in StructLayout documentation say it's for controlling the layout to pass a type to unmanaged code, i.e. match the layout of the unmanaged type, and it doesn't mention any of it.

I don't think we explicitly document this part of the rules anywhere (and as you can see in #71711, the semantics were not clear even to ourselves for a long time). I wouldn't be surprised if CUSTOMLAYOUT came to exist quite organically when some issue was noticed where the JIT discarded padding that was expected to be preserved.

[tomeksowi]

I believe most of your example test cases have discardable padding since they do not have ExplicitLayout/explicit size, so there I think the ABI classification done here makes sense. But for the example given above things are different, where the ABI classification should essentially be done as if the padding was replaced by explicit char arrays of the right size.

Right, the bulk of this PR is about empty struct fields, I'll leave out the ExplicitLayout test cases until we hammer out what to do with it, then I'll address it in a dedicated PR.

At any rate, the condition for which padding is preservable needs to be communicated loud and clear because it may change how the argument is passed. I think the best course of action would be to specify something like "When <condition for significant padding>, the field with its padding until the next field is treated as a fixed array of type same as the field" and then let the platform ABI decide how to pass a struct with that additional array so we're not inventing a custom ABI.

[jkotas]

I think there are both .NET structs (like the ones with ExplicitLayout/Size) that are not representable in C/C++; and C/C++ structs (like your example) that are not representable in .NET metadata.

Yes, that sounds about right.

[AaronRobinsonMSFT]

I think there are both .NET structs (like the ones with ExplicitLayout/Size) that are not representable in C/C++; and C/C++ structs (like your example) that are not representable in .NET metadata.

Agree.

[dotnet-policy-service]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.