Fix relocs for linux-riscv64 AOT #112331

am11 · 2025-02-10T02:47:54Z

With this delta, we successfully reach the hello-world milestone.

tomeksowi · 2025-02-10T08:23:28Z

src/coreclr/jit/codegenriscv64.cpp

+            GenTreeIntCon* con = op2->AsIntCon();
+
+            emitAttr attr = emitActualTypeSize(op2Type);
+            // TODO-CQ: Currently we cannot do this for all handles because of
+            // https://github.com/dotnet/runtime/issues/60712
+            if (con->ImmedValNeedsReloc(compiler))
+            {
+                attr = EA_SET_FLG(attr, EA_CNS_RELOC_FLG);
+            }
+
+            if (op2Type == TYP_BYREF)
+            {
+                attr = EA_SET_FLG(attr, EA_BYREF_FLG);
+            }
+
+            instGen_Set_Reg_To_Imm(attr, REG_RA, imm,
+                                   INS_FLAGS_DONT_CARE DEBUGARG(con->gtTargetHandle) DEBUGARG(con->gtFlags));
+            regSet.verifyRegUsed(REG_RA);


Nit: looks like the CNS_INT case does the same (it's unused now). Can we just call genSetRegToConst?

runtime/src/coreclr/jit/codegenriscv64.cpp

Lines 1057 to 1085 in 02f99e5

void CodeGen::genSetRegToConst(regNumber targetReg, var_types targetType, GenTree* tree)

{

switch (tree->gtOper)

{

case GT_CNS_INT:

{

// relocatable values tend to come down as a CNS_INT of native int type

// so the line between these two opcodes is kind of blurry

GenTreeIntCon* con = tree->AsIntCon();

ssize_t cnsVal = con->IconValue();

emitAttr attr = emitActualTypeSize(targetType);

// TODO-RISCV64-CQ: Currently we cannot do this for all handles because of

// https://github.com/dotnet/runtime/issues/60712

if (con->ImmedValNeedsReloc(compiler))

{

attr = EA_SET_FLG(attr, EA_CNS_RELOC_FLG);

}

if (targetType == TYP_BYREF)

{

attr = EA_SET_FLG(attr, EA_BYREF_FLG);

}

instGen_Set_Reg_To_Imm(attr, targetReg, cnsVal,

INS_FLAGS_DONT_CARE DEBUGARG(con->gtTargetHandle) DEBUGARG(con->gtFlags));

regSet.verifyRegUsed(targetReg);

}

break;

The difference is static_cast uint vs. int. If the value returned by op2->AsIntCon()->gtIconVal is negative, cond.IsUnsigned() is true, then cast to uint32_t will reinterpret it as a large positive number and cast to int32_t in else will keep it negative.

I can't tell if ignoring the signedness has any side-effects in this context.

The relocatable constants should be pointer sized, so they will not be going through the EA_4BYTE case with the casts above.

@jakobbotsch, instGen_Set_Reg_To_Imm(attr, REG_RA, imm, is called for both EA_4BYTE and EA_8BYTE right?

Ah, yes. It seems like a RISCV64 version of genSetRegToConst would need to come with an extra argument about whether 32-bit constants should be sign or zero extended given that the full 64-bit register is expected to be used by callers afterwards.
So I agree it probably can't be used directly here.

Yeah, let's leave it. Technically you could ABI-extend both sides (sign-extend from 32-bits up to full register, regardless of type's signedness), then the unsigned comparison on the whole register works correctly.

I hope to get to it this year, we've got plenty of unnecessary extension instructions, e.g. the register operand here is already ABI-extended in many cases.

risc-vv · 2025-02-10T12:34:36Z

13cfb9d is being scheduled for building and testing

GIT: 13cfb9d902564ec3ed85c2feb320bbe14ebd2d70
REPO: dotnet/runtime
BRANCH: main

Release-FX-tests FAILED

buildinfo.json
coreFX tests failed for unknown reason. Check buildinfo.json or gocd for more details.

jakobbotsch

Handles can show up in a number of JIT nodes. It seems likely a similar fix will be needed in other places too.

* main: Code clean up in AP for NonNull* (dotnet#112027) JIT: Invalidate LSRA's DFS tree if we aren't running new layout phase (dotnet#112364) Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250204.2 (dotnet#112339) Add doc on OS onboarding (dotnet#112026) Add `TypeName` APIs to simplify metadata lookup. (dotnet#111598) Internal monitor impl not using coop mutex causing deadlocks on Android. (dotnet#112358) Do not run NAOT arm64 OSX testing on all PRs (dotnet#112342) Special-case empty enumerables in AsyncEnumerable (dotnet#112321) Have mono handle ConvertToIntegerNative for Double and Single (dotnet#112206) Update dependencies from https://github.com/dotnet/arcade build 20250206.4 (dotnet#112338) System.Configuration.ConfigurationManager.Tests: use Assembly.Location to determine ThisApplicationPath. (dotnet#112231) Force write of local file header when "version needed to extract" changes (dotnet#112032) JIT: Don't reorder handler blocks (dotnet#112292) [RISC-V] Synthesize some floating constants inline (dotnet#111529) Enable `SA1000`: Spacing around keywords (dotnet#112302) Fix relocs for linux-riscv64 AOT (dotnet#112331)

Fix relocs for linux-riscv64 AOT

13cfb9d

am11 requested a review from MichalStrehovsky as a code owner February 10, 2025 02:47

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

am11 requested review from filipnavara and a team February 10, 2025 02:48

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 10, 2025

am11 added the arch-riscv Related to the RISC-V architecture label Feb 10, 2025

am11 mentioned this pull request Feb 10, 2025

RISC-V NativeAOT port #106223

Open

am11 added area-NativeAOT-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Feb 10, 2025

filipnavara approved these changes Feb 10, 2025

View reviewed changes

tomeksowi approved these changes Feb 10, 2025

View reviewed changes

MichalStrehovsky added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-NativeAOT-coreclr labels Feb 10, 2025

This was referenced Feb 10, 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 approved these changes Feb 10, 2025

View reviewed changes

jakobbotsch merged commit 2240175 into dotnet:main Feb 10, 2025
115 of 119 checks passed

am11 deleted the feature/nativeaot/riscv64-port branch February 10, 2025 13:02

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix relocs for linux-riscv64 AOT #112331

Fix relocs for linux-riscv64 AOT #112331

Uh oh!

am11 commented Feb 10, 2025

Uh oh!

tomeksowi Feb 10, 2025 •

edited

Loading

Uh oh!

am11 Feb 10, 2025

Uh oh!

jakobbotsch Feb 10, 2025

Uh oh!

am11 Feb 10, 2025

Uh oh!

jakobbotsch Feb 10, 2025

Uh oh!

tomeksowi Feb 10, 2025

Uh oh!

risc-vv commented Feb 10, 2025 •

edited

Loading

Uh oh!

jakobbotsch left a comment

Uh oh!

Uh oh!

Uh oh!

	void CodeGen::genSetRegToConst(regNumber targetReg, var_types targetType, GenTree* tree)
	{
	switch (tree->gtOper)
	{
	case GT_CNS_INT:
	{
	// relocatable values tend to come down as a CNS_INT of native int type
	// so the line between these two opcodes is kind of blurry
	GenTreeIntCon* con = tree->AsIntCon();
	ssize_t cnsVal = con->IconValue();

	emitAttr attr = emitActualTypeSize(targetType);
	// TODO-RISCV64-CQ: Currently we cannot do this for all handles because of
	// https://github.com/dotnet/runtime/issues/60712
	if (con->ImmedValNeedsReloc(compiler))
	{
	attr = EA_SET_FLG(attr, EA_CNS_RELOC_FLG);
	}

	if (targetType == TYP_BYREF)
	{
	attr = EA_SET_FLG(attr, EA_BYREF_FLG);
	}

	instGen_Set_Reg_To_Imm(attr, targetReg, cnsVal,
	INS_FLAGS_DONT_CARE DEBUGARG(con->gtTargetHandle) DEBUGARG(con->gtFlags));
	regSet.verifyRegUsed(targetReg);
	}
	break;

Fix relocs for linux-riscv64 AOT #112331

Fix relocs for linux-riscv64 AOT #112331

Uh oh!

Conversation

am11 commented Feb 10, 2025

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am11 Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

am11 Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

tomeksowi Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

risc-vv commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomeksowi Feb 10, 2025 •

edited

Loading

risc-vv commented Feb 10, 2025 •

edited

Loading