JIT: Remove uses of old ABI information #112818

jakobbotsch · 2025-02-22T16:54:31Z

Remove uses of compArgSize. This value is computed by the old LclVarDsc ABI classification code and used to derive the amount of stack spaced consumed for parameters. Replace it with the value from the new ABI information instead.
Remove LclVarDsc::lvSize and LclVarDsc::lvArgStackSize. Generally LclVarDsc::lvExactSize is the right replacement, unless specifically the stack home size is needed, in which case the replacement is Compiler::lvaLclStackHomeSize.
Remove uses of LclVarDsc HFA information. This was the primary motivation for removing LclVarDsc::lvSize since it was querying HFA information.

This value is computed by the old LclVarDsc ABI classification code and used to derive the amount of stack spaced consumed for parameters. Replace it with this value from the new ABI information instead.

dotnet-policy-service · 2025-02-22T16:55:07Z

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

jakobbotsch · 2025-02-22T16:56:17Z

src/coreclr/jit/codegencommon.cpp

-        if (isFramePointerUsed())
-        {
-#if defined(TARGET_ARM)
-            // GetStackOffset() is always valid for incoming stack-arguments, even if the argument
-            // will become enregistered.
-            // On Arm compiler->compArgSize doesn't include r11 and lr sizes and hence we need to add 2*REGSIZE_BYTES
-            noway_assert((2 * REGSIZE_BYTES <= varDsc->GetStackOffset()) &&
-                         (size_t(varDsc->GetStackOffset()) < compiler->compArgSize + 2 * REGSIZE_BYTES));
-#else
-            // GetStackOffset() is always valid for incoming stack-arguments, even if the argument
-            // will become enregistered.
-            noway_assert((0 < varDsc->GetStackOffset()) && (size_t(varDsc->GetStackOffset()) < compiler->compArgSize));
-#endif
-        }


We could replace compArgSize here as well, but the assert did not look useful to me.

am11 · 2025-02-22T21:22:25Z

src/coreclr/tools/superpmi/superpmi/jitinstance.cpp

+        GUID expected = JITEEVersionIdentifier;
+        GUID actual = versionId;
        LogError("Jit Compiler has wrong version identifier");


utilcode.h provides a conversion helper:

Suggested change

GUID expected = JITEEVersionIdentifier;

GUID actual = versionId;

LogError("Jit Compiler has wrong version identifier");

char expected[GUID_STR_BUFFER_LEN], actual[GUID_STR_BUFFER_LEN];

GuidToLPSTR(JITEEVersionIdentifier, expected, GUID_STR_BUFFER_LEN);

GuidToLPSTR(versionId, actual, GUID_STR_BUFFER_LEN));

LogError("Jit Compiler has wrong version identifier. expected: %s, actual: %s", expected, actual);

Thanks, but we don't link with utilcode from here. Looks like that's intentional:

runtime/src/coreclr/tools/superpmi/superpmi-shared/spmiutil.cpp

Lines 279 to 282 in 6022adf

// The following functions are used for arm64/arm32 relocation processing.

// They are copies of the code in src\coreclr\utilcode\util.cpp.

// We decided to copy them instead of linking with utilcode library

// to avoid introducing additional runtime dependencies.

(I removed the leftover code from this PR, but #112820 has the same change.)

Actually we do link with it, but only on Linux it seems. I will just add it to Windows too.

That does seem to pull in more dependencies that we aren't linking with.. probably something to do separately at this point.

Maybe we can move the formatting code where GUID is defined:

src/native/minipal/guid.h

#define GUID_STR_BUFFER_LEN (ARRAY_SIZE("{12345678-1234-1234-1234-123456789abc}")) int32_t GUIDAsString(GUID guid, char* guidString, uint32_t len) { assert(len >= GUID_STR_BUFFER_LEN); return snprintf(guidString, len, "{%08lx-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x}", guid.data1, guid.data2, guid.data3, guid.data4[0], guid.data4[1], guid.data4[2], guid.data4[3], guid.data4[4], guid.data4[5], guid.data4[6], guid.data4[7]) + 1; }

and remove duplicate definitions from src/coreclr/inc/jiteeversionguid.h, src/coreclr/pal/inc/pal_mstypes.h and src/tests/Interop/common/xplatform.h.

I think that's best left for a different PR.

Agreed, I've opened #112826.

…compArgSize

jakobbotsch · 2025-02-23T21:56:56Z

cc @dotnet/jit-contrib PTAL @EgorBo

Diffs. There are a few diffs on osx-arm64 where float HFAs are unaligned when passed as stack parameters (e.g. a 12 byte float HFA consumes exactly 12 bytes, and the next parameter can start right after it). The new lvaLclStackHomeSize will detect this case accurately and round up the size of other float HFAs, while the old logic would be more conservative and never round the size up in these cases.

Also some minor TP diffs. After this PR we should be able to get rid of the old parameter ABI classification which should result in some TP improvements.

EgorBo

wonder if it fixes one of the HFA issues assigned to me, I'll check

JIT: Remove uses of compArgSize

7ae27b5

This value is computed by the old LclVarDsc ABI classification code and used to derive the amount of stack spaced consumed for parameters. Replace it with this value from the new ABI information instead.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 22, 2025

dotnet-policy-service bot assigned jakobbotsch Feb 22, 2025

jakobbotsch commented Feb 22, 2025

View reviewed changes

jakobbotsch added 4 commits February 22, 2025 21:28

WIP

f8c6678

Remove assert

c6acd02

Remove another use

99d4f39

Run jit-format

9ceb9ba

jakobbotsch changed the title ~~JIT: Remove uses of compArgSize~~ JIT: Remove uses of old ABI information Feb 22, 2025

jakobbotsch added 3 commits February 22, 2025 21:33

Revert

9c41343

Fix LA64 lvSize uses

a9d8b95

Fix LA64 build

e359041

am11 reviewed Feb 22, 2025

View reviewed changes

Remove leftover code

609ce24

This was referenced Feb 23, 2025

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

jakobbotsch added 2 commits February 23, 2025 11:33

Merge branch 'main' of github.com:dotnet/runtime into remove-uses-of-…

d85a5dc

…compArgSize

Fix quirk for stack "HFA" parameters

c0b12d8

jakobbotsch marked this pull request as ready for review February 23, 2025 21:50

jakobbotsch requested a review from EgorBo February 23, 2025 21:57

EgorBo approved these changes Feb 24, 2025

View reviewed changes

jakobbotsch merged commit 9be0be0 into dotnet:main Feb 24, 2025
112 of 115 checks passed

jakobbotsch deleted the remove-uses-of-compArgSize branch February 24, 2025 16:18

github-actions bot locked and limited conversation to collaborators Mar 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JIT: Remove uses of old ABI information #112818

JIT: Remove uses of old ABI information #112818

Uh oh!

jakobbotsch commented Feb 22, 2025 •

edited

Loading

Uh oh!

dotnet-policy-service bot commented Feb 22, 2025

Uh oh!

jakobbotsch Feb 22, 2025

Uh oh!

am11 Feb 22, 2025

Uh oh!

jakobbotsch Feb 22, 2025

Uh oh!

jakobbotsch Feb 22, 2025

Uh oh!

jakobbotsch Feb 22, 2025

Uh oh!

am11 Feb 22, 2025

Uh oh!

jakobbotsch Feb 23, 2025

Uh oh!

am11 Feb 23, 2025

Uh oh!

jakobbotsch commented Feb 23, 2025

Uh oh!

EgorBo left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-        GUID expected = JITEEVersionIdentifier;
-        GUID actual = versionId;
-        LogError("Jit Compiler has wrong version identifier");
+        char expected[GUID_STR_BUFFER_LEN], actual[GUID_STR_BUFFER_LEN];
+        GuidToLPSTR(JITEEVersionIdentifier, expected, GUID_STR_BUFFER_LEN);
+        GuidToLPSTR(versionId, actual, GUID_STR_BUFFER_LEN));
+        LogError("Jit Compiler has wrong version identifier. expected: %s, actual: %s", expected, actual);

	// The following functions are used for arm64/arm32 relocation processing.
	// They are copies of the code in src\coreclr\utilcode\util.cpp.
	// We decided to copy them instead of linking with utilcode library
	// to avoid introducing additional runtime dependencies.

JIT: Remove uses of old ABI information #112818

JIT: Remove uses of old ABI information #112818

Uh oh!

Conversation

jakobbotsch commented Feb 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Feb 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Feb 23, 2025

Uh oh!

EgorBo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jakobbotsch commented Feb 22, 2025 •

edited

Loading