Skip to content

Fix relocs for linux-riscv64 AOT #112331

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

Merged
merged 1 commit into from
Feb 10, 2025
Merged

Fix relocs for linux-riscv64 AOT #112331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 18 additions & 1 deletion src/coreclr/jit/codegenriscv64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3581,7 +3581,24 @@ void CodeGen::genCodeForJumpCompare(GenTreeOpCC* tree)
unreached();
}

emit->emitLoadImmediate(EA_PTRSIZE, REG_RA, imm);
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);
Comment on lines +3584 to +3601
Copy link
Contributor

@tomeksowi tomeksowi Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

regs = (int)REG_RA << 5;
}
else
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public void EmitRETIfZero(Register regSrc)

public void EmitJMPIfZero(Register regSrc, ISymbolNode symbol)
{
uint offset = symbol.RepresentsIndirectionCell ? 28u : 8u;
uint offset = symbol.RepresentsIndirectionCell ? 28u : 12u;
uint encodedOffset = ((offset & 0x1e) << 7) | ((offset & 0x7e0) << 20) | ((offset & 0x800) >> 4) | ((offset & 0x1000) << 19);
// bne regSrc, x0, offset
Builder.EmitUInt((uint)(0x00001063 | ((uint)regSrc << 15) | encodedOffset));
Expand Down
Loading