Skip to content
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.

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

[RISC-V] Use common CodeGen::genHomeRegisterParams #101288

Merged
merged 16 commits into from
Apr 27, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
14 changes: 8 additions & 6 deletions src/coreclr/jit/abi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,16 @@ bool ABIPassingInformation::HasExactlyOneStackSegment() const
//
bool ABIPassingInformation::IsSplitAcrossRegistersAndStack() const
{
bool anyReg = false;
bool anyStack = false;
for (unsigned i = 0; i < NumSegments; i++)
if (NumSegments < 2)
return false;

bool isFirstInReg = Segments[0].IsPassedInRegister();
for (unsigned i = 1; i < NumSegments; i++)
{
anyReg |= Segments[i].IsPassedInRegister();
anyStack |= Segments[i].IsPassedOnStack();
if (isFirstInReg != Segments[i].IsPassedInRegister())
return true;
}
return anyReg && anyStack;
return false;
}

//-----------------------------------------------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/abi.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ struct ABIPassingInformation
// - On loongarch64/riscv64, structs can be passed in two registers or
// can be split out over register and stack, giving
// multiple register segments and a struct segment.
unsigned NumSegments = 0;
ABIPassingSegment* Segments = nullptr;
unsigned NumSegments;
ABIPassingSegment* Segments;

ABIPassingInformation(unsigned numSegments = 0, ABIPassingSegment* segments = nullptr)
: NumSegments(numSegments)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/codegen.h
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ class CodeGen final : public CodeGenInterface
void genEnregisterOSRArgsAndLocals();
#endif

void genHomeSwiftStructParameters(bool handleStack);
void genHomeStackParams(bool handleStack);

void genCheckUseBlockInit();
#if defined(UNIX_AMD64_ABI) && defined(FEATURE_SIMD)
Expand Down
30 changes: 20 additions & 10 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2802,7 +2802,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
*/

#if !defined(TARGET_RISCV64)
struct RegNode;

struct RegNodeEdge
Expand Down Expand Up @@ -3346,8 +3345,6 @@ void CodeGen::genHomeRegisterParams(regNumber initReg, bool* initRegStillZeroed)
}
}

#endif

// -----------------------------------------------------------------------------
// genGetParameterHomingTempRegisterCandidates: Get the registers that are
// usable during register homing.
Expand Down Expand Up @@ -4122,32 +4119,34 @@ void CodeGen::genEnregisterOSRArgsAndLocals()
}
}

#ifdef SWIFT_SUPPORT
#if defined(SWIFT_SUPPORT) || defined(TARGET_RISCV64)

//-----------------------------------------------------------------------------
// genHomeSwiftStructParameters:
// Reassemble Swift struct parameters if necessary.
// genHomeStackParams:
// Reassemble parameters if necessary.
//
// Parameters:
// handleStack - If true, reassemble the segments that were passed on the stack.
// If false, reassemble the segments that were passed in registers.
//
void CodeGen::genHomeSwiftStructParameters(bool handleStack)
void CodeGen::genHomeStackParams(bool handleStack)
{
for (unsigned lclNum = 0; lclNum < compiler->info.compArgsCount; lclNum++)
{
#ifdef SWIFT_SUPPORT
if (lclNum == compiler->lvaSwiftSelfArg)
{
continue;
}
#endif

LclVarDsc* dsc = compiler->lvaGetDesc(lclNum);
if ((dsc->TypeGet() != TYP_STRUCT) || compiler->lvaIsImplicitByRefLocal(lclNum) || !dsc->lvOnFrame)
{
continue;
}

JITDUMP("Homing Swift parameter V%02u: ", lclNum);
JITDUMP("Homing reassembled parameter V%02u: ", lclNum);
const ABIPassingInformation& abiInfo = compiler->lvaGetParameterABIInfo(lclNum);
DBEXEC(VERBOSE, abiInfo.Dump());

Expand Down Expand Up @@ -4185,9 +4184,13 @@ void CodeGen::genHomeSwiftStructParameters(bool handleStack)
case 2:
loadType = TYP_USHORT;
break;
case 3:
case 4:
loadType = TYP_INT;
break;
case 5:
case 6:
case 7:
case 8:
loadType = TYP_LONG;
break;
Expand Down Expand Up @@ -4224,7 +4227,7 @@ void CodeGen::genHomeSwiftStructParameters(bool handleStack)
}
}
}
#endif
#endif // defined(SWIFT_SUPPORT) || defined(TARGET_RISCV64)
Copy link
Member

@jakobbotsch jakobbotsch Apr 25, 2024

Choose a reason for hiding this comment

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

Isn't this going to result in a lot of extra stack stores for RISCV?
At this point it seems like just factoring parts of this helper out and calling it from the same place as genHomeRegisterParams on the split stack segment will end up being a simpler change overall.

Copy link
Contributor Author

@tomeksowi tomeksowi Apr 26, 2024

Choose a reason for hiding this comment

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

It didn't make unnecessary stores from what I examined the diffs but you're right, a RISC-V specific routine (I tried to phrase it so LoongArch can also reuse) turned out much handier.


/*-----------------------------------------------------------------------------
*
Expand Down Expand Up @@ -5516,7 +5519,7 @@ void CodeGen::genFnProlog()
intRegState.rsCalleeRegArgMaskLiveIn &= ~RBM_SWIFT_ERROR;
}

genHomeSwiftStructParameters(/* handleStack */ false);
genHomeStackParams(/* handleStack */ false);
}
#endif

Expand Down Expand Up @@ -5552,6 +5555,13 @@ void CodeGen::genFnProlog()
genEnregisterIncomingStackArgs();
}

#ifdef TARGET_RISCV64
if (compiler->fgFirstBBisScratch() && compiler->lvaHasAnyStackParamToReassemble())
{
genHomeStackParams(/* handleStack */ true);
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

The scratch BB is not necessary when it happens during the prolog.

I think it would be better to do this before you home register parameters (in the non-OSR case above), and passing initReg/initRegZeroed as well so there is no chance of overwriting a homed register.


/* Initialize any must-init registers variables now */

if (initRegs)
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/jit/codegenlinear.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -387,12 +387,12 @@ void CodeGen::genCodeForBBlist()
compiler->compCurLifeTree = nullptr;

#ifdef SWIFT_SUPPORT
// Reassemble Swift struct parameters on the local stack frame in the
// Reassemble split struct parameters on the local stack frame in the
// scratch BB right after the prolog. There can be arbitrary amounts of
// codegen related to doing this, so it cannot be done in the prolog.
if (compiler->fgBBisScratch(block) && compiler->lvaHasAnySwiftStackParamToReassemble())
if (compiler->fgBBisScratch(block) && compiler->lvaHasAnyStackParamToReassemble())
{
genHomeSwiftStructParameters(/* handleStack */ true);
genHomeStackParams(/* handleStack */ true);
}
#endif

Expand Down
Loading