[RISC-V] ELT Profiler Bring-Up #91313

tomeksowi · 2023-08-30T09:03:17Z

Initial implementation of ELT profiler for RISC-V. Fixes tests:

profiler/elt/slowpatheltenter/slowpatheltenter.sh
profiler/elt/slowpatheltleave/slowpatheltleave.sh
profiler/unittest/inlining/inlining.sh

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03

@jkotas @jakobbotsch

Initial implementation based on ARM64 code.

* Fix argument registers according to RISC-V calling convention * Fix field offsets for PROFILE_PLATFORM_SPECIFIC_DATA * Make sure field offsets for PROFILE_PLATFORM_SPECIFIC_DATA stay fixed by static asserting the offsets in asmconstants.h

…in t0 and t1 because t2 is used to store the call address of the stub

…n see whole struct arguments laid out in memory

Since the RISC-V ABI says values are returned like the first named argument, re-use the struct copying routine from argument parsing as much as possible.

…filer::Shutdown()

* Remove unused t0 field * Remove 'unused' field and widen 'flags' to 64 bits to maintain alignment and shave off one sw instruction

ghost · 2023-08-30T09:03:31Z

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

Issue Details

Initial implementation of ELT profiler for RISC-V. Fixes tests:

profiler/elt/slowpatheltenter/slowpatheltenter.sh
profiler/elt/slowpatheltleave/slowpatheltleave.sh
profiler/unittest/inlining/inlining.sh

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov @clamp03

@jkotas @jakobbotsch

Author:	tomeksowi
Assignees:	-
Labels:	`area-Diagnostics-coreclr`, `community-contribution`
Milestone:	-

tomeksowi · 2023-08-30T14:54:18Z

@jkotas @jakobbotsch PR ready for review.

clamp03 · 2023-08-31T02:23:21Z

src/coreclr/vm/riscv64/cgencpu.h

+// Profiling
+//**********************************************************************
+
+#ifdef PROFILING_SUPPORTED


IMO, it is better to put to original vm/riscv64/profiler.cpp back for consistency with other platforms.

Putting it back in profiler.cpp also means we're back to hardcoded field offsets in asm stubs, which are easy to get wrong. Using constants pinned to the actual C struct by static asserts in asmconstants.h is a net improvement, even though the code differs somewhat from other platforms.

clamp03 · 2023-08-31T02:40:14Z

src/coreclr/vm/riscv64/profiler.cpp

-            return (LPVOID)pData->argumentRegisters.a[0];
-        }
+        // On RISC-V the method is not required to preserve the return buffer address passed in a0.
+        // However, JIT does that anyway if leave hook needs to be generated.


Please put the assert _ASSERTE((pData->flags & PROFILE_LEAVE) != 0); if possible.

clamp03 · 2023-08-31T03:04:45Z

src/coreclr/jit/codegenriscv64.cpp

+    if (compiler->compProfilerMethHndIndirected)
+    {
+        instGen_Set_Reg_To_Imm(EA_PTR_DSP_RELOC, REG_PROFILER_ENTER_ARG_FUNC_ID, methHnd);
+        GetEmitter()->emitIns_R_R(INS_ld, EA_PTRSIZE, REG_PROFILER_ENTER_ARG_FUNC_ID, REG_PROFILER_ENTER_ARG_FUNC_ID);


In RISC-V, emitIns_R_R_I is used for INS_ld

Good point, will do.

clamp03 · 2023-08-31T03:05:09Z

src/coreclr/jit/codegenriscv64.cpp

+    if (compiler->compProfilerMethHndIndirected)
+    {
+        instGen_Set_Reg_To_Imm(EA_PTR_DSP_RELOC, REG_PROFILER_LEAVE_ARG_FUNC_ID, methHnd);
+        GetEmitter()->emitIns_R_R(INS_ld, EA_PTRSIZE, REG_PROFILER_LEAVE_ARG_FUNC_ID, REG_PROFILER_LEAVE_ARG_FUNC_ID);


Please replace to emitIns_R_R_I

clamp03 · 2023-08-31T03:17:23Z

src/coreclr/vm/riscv64/asmconstants.h

+#undef ASMCONSTANTS_C_ASSERT_OFFSET
+
+#endif // PROFILING_SUPPORTED
+


It does not sync with other plaforms. IMO, I think it is better to update with all other platforms in this case. @jkotas

Like in the other comment on cgencpu.h, IMO having the field offsets pinned to the C struct with static asserts is a net improvement over hardcoded offsets in asm.

I wanted to limit the impact of the change to RISC-V since this PR is large enough. But I can do the same for other platforms if need be.

Okay. I understand your concerns. However, I prefer that the same structures and functions should be in the same file for all platforms as much as possible. So I want you to refactoring all the other platforms in another PR. (or you can focus bug-fix only in this PR. Then updates all (including RISC-V) in another PR.) Thank you.
@jkotas Could you give any comment about this?

I agree that it would be nice to use named constants that are validated at compile time, across all platforms. This cleanup can be done in a separate PR.

OK, I'll do an analogous cleanup for PROFILE_PLATFORM_SPECIFIC_DATA in a separate PR.

clamp03 · 2023-08-31T05:02:17Z

src/coreclr/vm/riscv64/profiler.cpp

+    PROFILE_PLATFORM_SPECIFIC_DATA* pData = reinterpret_cast<PROFILE_PLATFORM_SPECIFIC_DATA*>(m_handle);
+
+    struct { bool isFloat, is8; } fields[] = {
+        { sir->m_structFields & (STRUCT_FLOAT_FIELD_FIRST | STRUCT_FLOAT_FIELD_ONLY_TWO | STRUCT_FLOAT_FIELD_ONLY_ONE),


In my guess, fields[0] is for the first one in the field. If then, I think STRUCT_FLOAT_FIELD_ONLY_ONE can be set when only second field is FLOAT in GetRiscv64PassStructInRegisterFlags. (I don't know it can actually happen. I just searched.) Could you check again?

runtime/src/coreclr/vm/methodtable.cpp

Lines 3783 to 3794 in 6d3be9e

int size2 = GetRiscv64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);

if ((size2 & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)

{

if (pFieldSecond[0].GetSize() == 8)

{

size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size | STRUCT_SECOND_FIELD_DOUBLE);

}

else

{

size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size | STRUCT_FLOAT_FIELD_SECOND);

}

}

It passes the MixedStructFunc test case from slowpathcommon.cs and it takes a struct with second float field only. But I'll check if it ever happens.

clamp03 · 2023-08-31T05:05:25Z

src/coreclr/vm/riscv64/profiler.cpp

+            m_bufferPos = alignedTo8;
+            const INT64* src =
+                inFloatReg ? (const INT64*)fReg++ :
+                inGenReg   ? aReg++ : (const INT64*)Func::postIncrement(stack, 8);


When field[i].isFloat is true and fReg < fRegEnd is false, src can be a aReg which is an integer register. Please check.

That's intentional to cover this case from RISC-V ABI, 2.2 Hardware Floating-point Calling Convention:

A real floating-point argument is passed in a floating-point argument register if it is no more than
ABI_FLEN bits wide and at least one floating-point argument register is available. Otherwise, it is
passed according to the integer calling convention.

clamp03 · 2023-08-31T05:57:50Z

src/coreclr/vm/riscv64/profiler.cpp

+        sir.m_byteStackIndex = 0;
+        sir.m_byteStackSize = -1;
+        sir.m_structFields = returnFlags;
+        return CopyStructFromRegisters(&sir);


I think in most cases except VALUE_TYPE, it can avoid CopyStructFromRegisters like you did in GetNextArgAddr. AndfpReturnSize looks better than returnFlags.

Good point, I'll avoid CopyStructFromRegisters if a pointer to argument registers can be returned.

clamp03 · 2023-08-31T06:05:19Z

src/coreclr/vm/riscv64/cgencpu.h

+    void*                  hiddenArg;
+    UINT64                 flags;
+    // Scratch space to reconstruct struct passed in registers
+    BYTE                   buffer[sizeof(ArgumentRegisters) + sizeof(FloatArgumentRegisters)];


Is it sufficient for all arguments and return? CopyStructFromRegisters can copy argument registers, return registers and stacks. Could you please check?

IMO, it will suffice. For return values we have a separate structure allocated by the ProfileLeaveNaked stub. So worst case scenario would be a function with 8 or more arguments of type struct LongDouble { long; double; } to use all argument registers for arguments needed to be reconstructed in scratch space.

I'll check the edge-cases where the last struct in registers needs to be partially on the stack, e.g. func(int i, LongDouble ld1 ... ld8) but in that case IMO we still fit within the limit.

alpencolt · 2023-08-31T12:37:03Z

src/coreclr/vm/riscv64/asmhelpers.S

-
-    sd    zero, 240(sp)        // Clear hiddenArg.
+    SAVE_ARGUMENT_REGISTERS sp, PROFILE_PLATFORM_SPECIFIC_DATA__argumentRegisters
+    sd     zero, PROFILE_PLATFORM_SPECIFIC_DATA__functionId(sp)


SIZEOF__PROFILE_PLATFORM_SPECIFIC_DATA is 80 but was 88, why it was changed? And in common why it's so?

I think you mean PROFILE_PLATFORM_SPECIFIC_DATA__functionId? The hitherto PROFILE_PLATFORM_SPECIFIC_DATA had a x8 field in front of it, which probably was a copy-paste from ARM64. We don't need it in RISC-V so I removed it and all the following fields moved up by 8 bytes.

Now all the offset constants are static asserted with the original C struct so it won't compile if there's a mismatch.

Yes, sorry, I mean PROFILE_PLATFORM_SPECIFIC_DATA__functionId.
What the purpose of x8?

According to ARM64 calling convention x8 stores the return buffer address. On RISC-V it's passed as an implicit first parameter, i.e. a0.

alpencolt · 2023-08-31T12:37:48Z

src/coreclr/vm/riscv64/asmhelpers.S

    addi  t6, zero, \flags
-    sw  t6, 248(sp)            // Save flags.
-    sw  zero, 252(sp)          // clear unused field.
+    sd  t6, PROFILE_PLATFORM_SPECIFIC_DATA__flags(sp)


And other offsets

Ditto my answer on PROFILE_PLATFORM_SPECIFIC_DATA__functionId.

alpencolt · 2023-08-31T13:05:44Z

src/coreclr/vm/riscv64/asmhelpers.S

-    //   t1 = functionIDOrClientID
-    //   t2 = profiledSp
+    //   t0 = functionIDOrClientID
+    //   t1 = profiledSp
    //   t6 = throwable


Can't find who set arguments. You've changed register, should it be updated?

The arguments are set in CodeGen::genProfiling(Enter|Leave)Callback in codegenriscv64.cpp. Yes, they should and are updated.

The reason I changed the registers is because t2 is used on RISC-V to store the call address of the stub by genEmitHelperCall.

clamp03 · 2023-09-01T02:01:34Z

src/coreclr/vm/riscv64/profiler.cpp

+    const double *fRegBegin = &pData->floatArgumentRegisters.f[sir->m_idxFloatReg], *fReg = fRegBegin;
+    const double *fRegEnd = fReg + sizeof(pData->floatArgumentRegisters.f)/sizeof(pData->floatArgumentRegisters.f[0]);
+    const INT64 *aRegBegin = &pData->argumentRegisters.a[sir->m_idxGenReg], *aReg = aRegBegin;
+    const INT64 *aRegEnd = aReg + sizeof(pData->argumentRegisters.a)/sizeof(pData->argumentRegisters.a[0]);


I think you can use NUM_ARGUMENT_REGISTERS and NUM_FLOAT_ARGUMENT_REGISTERS.
And updates ArgumentRegisters and FloatArgumentRegisters structures to use the definitions instead of number 8.

clamp03 · 2023-09-01T09:01:57Z

src/coreclr/vm/riscv64/profiler.cpp

+    const double *fRegBegin = &pData->floatArgumentRegisters.f[sir->m_idxFloatReg], *fReg = fRegBegin;
+    const double *fRegEnd = fReg + NUM_FLOAT_ARGUMENT_REGISTERS;
+    const INT64 *aRegBegin = &pData->argumentRegisters.a[sir->m_idxGenReg], *aReg = aRegBegin;
+    const INT64 *aRegEnd = aReg + NUM_ARGUMENT_REGISTERS;


It looks like aRegEnd = &pData->argumentRegisters.a[0] + NUM_ARGUMENT_REGISTERS and fRegEnd = &pData->floatArgumentRegisters.f[0] + NUM_FLOAT_ARGUMENT_REGISTERS are correct. Could you check?

Of course, thanks!

clamp03

Thank you so much!!!

tomeksowi · 2023-09-04T15:17:11Z

@davmason @jakobbotsch could you please review?

jakobbotsch

The JIT changes look ok to me.

davmason

The profiler specific changes look good to me, I don't know enough about risc-v or the jit to comment on those changes.

jkotas · 2023-09-05T20:22:29Z

Thank you all

tomeksowi added 9 commits August 22, 2023 17:54

[RISC-V] Generate profiling function callbacks

e76256b

Initial implementation based on ARM64 code.

[RISC-V] Pass arguments for Profile(Enter|Leave|Tailcall)Naked stubs …

91e9bf1

…in t0 and t1 because t2 is used to store the call address of the stub

[RISC-V] Remove unimplemented definition of EmitRet

2b0d171

[RISC-V] Copy struct from registers into a buffer so that profiler ca…

b38930c

…n see whole struct arguments laid out in memory

[RISC-V] Implement ProfileArgIterator::GetReturnBufferAddr()

9ca5447

Since the RISC-V ABI says values are returned like the first named argument, re-use the struct copying routine from argument parsing as much as possible.

Factor out duplicated test results checking routine in SlowPathELTPro…

56a9863

…filer::Shutdown()

[RISC-V] Clean up PROFILE_PLATFORM_SPECIFIC_DATA

90c6b3a

* Remove unused t0 field * Remove 'unused' field and widen 'flags' to 64 bits to maintain alignment and shave off one sw instruction

[RISC-V] Remove commented out code

e33d81e

dotnet-issue-labeler bot added the area-Diagnostics-coreclr label Aug 30, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 30, 2023

[RISC-V] Fix formatting

c94bae2

clamp03 added the arch-riscv Related to the RISC-V architecture label Aug 30, 2023

clamp03 assigned tomeksowi Aug 30, 2023

[RISC-V] Apply format patch from failed check

fb982b5

jkotas requested review from davmason, jakobbotsch, alpencolt and clamp03 August 30, 2023 16:14

clamp03 suggested changes Aug 31, 2023

View reviewed changes

alpencolt reviewed Aug 31, 2023

View reviewed changes

alpencolt approved these changes Aug 31, 2023

View reviewed changes

clamp03 reviewed Sep 1, 2023

View reviewed changes

tomeksowi requested a review from clamp03 September 1, 2023 08:39

clamp03 reviewed Sep 1, 2023

View reviewed changes

[RISC-V] Post-review fixes

1f46d7c

tomeksowi force-pushed the elt-profiler branch from 8a229bf to 1f46d7c Compare September 1, 2023 13:12

build-analysis bot mentioned this pull request Sep 1, 2023

Microsoft.NET.HostModel.Tests failing with "No space left on device" #91039

Closed

clamp03 approved these changes Sep 3, 2023

View reviewed changes

jakobbotsch approved these changes Sep 5, 2023

View reviewed changes

tomeksowi mentioned this pull request Sep 5, 2023

Validate hardcoded offsets to PROFILE_PLATFORM_SPECIFIC_DATA struct #91595

Merged

davmason approved these changes Sep 5, 2023

View reviewed changes

jkotas merged commit 913a844 into dotnet:main Sep 5, 2023

ghost locked as resolved and limited conversation to collaborators Oct 6, 2023

		#undef ASMCONSTANTS_C_ASSERT_OFFSET

		#endif // PROFILING_SUPPORTED

	int size2 = GetRiscv64PassStructInRegisterFlags((CORINFO_CLASS_HANDLE)pMethodTable);
	if ((size2 & STRUCT_FLOAT_FIELD_ONLY_ONE) != 0)
	{
	if (pFieldSecond[0].GetSize() == 8)
	{
	size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND_8) : (size \| STRUCT_SECOND_FIELD_DOUBLE);
	}
	else
	{
	size = size & STRUCT_FLOAT_FIELD_FIRST ? (size ^ STRUCT_MERGE_FIRST_SECOND) : (size \| STRUCT_FLOAT_FIELD_SECOND);
	}
	}

[RISC-V] ELT Profiler Bring-Up #91313

[RISC-V] ELT Profiler Bring-Up #91313

Conversation

tomeksowi commented Aug 30, 2023

ghost commented Aug 30, 2023

tomeksowi commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpencolt Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 left a comment

Choose a reason for hiding this comment

tomeksowi commented Sep 4, 2023

jakobbotsch left a comment

Choose a reason for hiding this comment

davmason left a comment

Choose a reason for hiding this comment

jkotas commented Sep 5, 2023

clamp03 Aug 31, 2023 •

edited

Loading

clamp03 Aug 31, 2023 •

edited

Loading

clamp03 Aug 31, 2023 •

edited

Loading

alpencolt Aug 31, 2023 •

edited

Loading