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

JIT: Add missing new-style ABI classifiers for LA64 and RISCV64 #100744

Closed
jakobbotsch opened this issue Apr 7, 2024 · 11 comments
Closed

JIT: Add missing new-style ABI classifiers for LA64 and RISCV64 #100744

jakobbotsch opened this issue Apr 7, 2024 · 11 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Apr 7, 2024

#100138 introduced a new representation for ABI information that I am planning on switching the JIT to use in all places that deal with ABI details. It explicitly represents how all parts of all parameters are passed. #100276 and #100526 introduced classifiers for all of our supported targets: win-x86, win-x64, SysV x64, arm64 and arm32.
#100572 is an example PR that is making use of the new ABI representation to rewrite parameter homing.

Before I can proceed with moving the rest of the JIT to the new representation we need implementations of new-style classifiers for LA64 (cc @shushanhf) and RISCV64 (cc @dotnet/samsung). I would greatly appreciate contributions of implementations of these. Otherwise I can try to base it on what happens in lvaInitUserArgs today, but I will not be able to test it.

Once implemented, the following code can be enabled to cross check the new style ABI information against the old one stored in LclVarDsc:

#if defined(TARGET_X86) || defined(TARGET_AMD64) || defined(TARGET_ARM64) || defined(TARGET_ARM)
{
PlatformClassifier classifier(cInfo);
lvaClassifyParameterABI(classifier);
}
#endif
#ifdef DEBUG
if (lvaParameterPassingInfo == nullptr)
{
return;
}
for (unsigned lclNum = 0; lclNum < info.compArgsCount; lclNum++)
{
LclVarDsc* dsc = lvaGetDesc(lclNum);
const ABIPassingInformation& abiInfo = lvaParameterPassingInfo[lclNum];
assert(abiInfo.NumSegments > 0);
if ((dsc->TypeGet() == TYP_STRUCT) && (info.compCallConv == CorInfoCallConvExtension::Swift))
{
continue;
}
unsigned numSegmentsToCompare = abiInfo.NumSegments;
if (dsc->lvIsHfa())
{
// LclVarDsc only has one register set for HFAs
numSegmentsToCompare = 1;
}
#ifdef TARGET_ARM
// On arm the old representation only represents the start register for
// struct multireg args.
if (varTypeIsStruct(dsc))
{
numSegmentsToCompare = 1;
}
// And also for TYP_DOUBLE on soft FP
if (opts.compUseSoftFP && (dsc->TypeGet() == TYP_DOUBLE))
{
numSegmentsToCompare = 1;
}
#endif
for (unsigned i = 0; i < numSegmentsToCompare; i++)
{
const ABIPassingSegment& expected = abiInfo.Segments[i];
regNumber reg = REG_NA;
if (i == 0)
{
reg = dsc->GetArgReg();
}
#if FEATURE_MULTIREG_ARGS
else if (i == 1)
{
reg = dsc->GetOtherArgReg();
}
#endif
if (expected.IsPassedOnStack())
{
if (i == 0)
{
assert(reg == REG_STK);
// On x86, varargs methods access stack args off of a base pointer, and the
// first stack arg is not considered to be at offset 0.
// TODO-Cleanup: Unify things so that x86 is consistent with other platforms
// here and change fgMorphExpandStackArgForVarArgs to account for that.
#ifndef TARGET_X86
assert((unsigned)dsc->GetStackOffset() == expected.GetStackOffset());
#endif
}
}
else
{
assert(reg == expected.GetRegister());
}
}
}
#endif // DEBUG

Eventually the old information will be removed from LclVarDsc and the new style information will be the only source-of-truth for the ABI information.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 7, 2024
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Apr 7, 2024
@clamp03
Copy link
Member

clamp03 commented Apr 8, 2024

@bartlomiejko I have to get off work early. So can someone in your team help RISCV64 implementation and tests?

@shushanhf
Copy link
Contributor

Thanks, I will study these new code.

@tomeksowi
Copy link
Contributor

@bartlomiejko I have to get off work early. So can someone in your team help RISCV64 implementation and tests?

I can do that once I'm done with #100080, it's a similar area.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Apr 9, 2024
@tomeksowi
Copy link
Contributor

@jakobbotsch If a sub-word value is passed, shouldn't ABIPassingSegment also contain info whether the unused bits are (sign|zero)-extended (or NaN-boxed) vs. undefined?

@jakobbotsch
Copy link
Member Author

@jakobbotsch If a sub-word value is passed, shouldn't ABIPassingSegment also contain info whether the unused bits are (sign|zero)-extended (or NaN-boxed) vs. undefined?

Do you think it would be useful? ARM32 has similar requirements in its ABI, but I don't immediately see where we would end up actually querying this information on ABIPassingSegment today.
I would probably wait with adding it until we see some more evidence of it being useful, or do some work to make use of it. Maybe that will show up naturally once we move more things to the new representation.

Another missing piece right now is information about when structs are passed by reference, but I'm also delaying adding that information until we move the things that actually query this information to the new representation.

@tomeksowi
Copy link
Contributor

Do you think it would be useful? ARM32 has similar requirements in its ABI, but I don't immediately see where we would end up actually querying this information on ABIPassingSegment today.

There's a bit of a discrepancy where e.g. an integer primitive is (sign|zero)-extended but for struct { float; int; } the unused integer bits are unspecified. But it could also be inferred by looking at whether the type is a struct or not, so it's more of a convenience than must-have.

I would probably wait with adding it until we see some more evidence of it being useful, or do some work to make use of it. Maybe that will show up naturally once we move more things to the new representation.

Makes sense, thanks.

@shushanhf
Copy link
Contributor

shushanhf commented Apr 17, 2024

@jakobbotsch
The new-style ABI is used to replace the Compiler::lvaInitUserArgs{} where resolve the args info, is it?
If we add the classifier for LA64/RV64, we will delete the Compiler::lvaInitUserArgs{} ?
IMO, I think we can uniform the classifier and the Compiler::lvaInitUserArgs{}, that is, can we cache the classifier info within the Compiler::lvaInitUserArgs{}

Or we just add the new-style ABI classifier for LA64 and RV64 first.
The optimization or amendment will be discussed later by separately PR?

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Apr 17, 2024

@jakobbotsch The new-style ABI is used to replace the Compiler::lvaInitUserArgs{} where resolve the args info, is it? If we add the classifier for LA64/RV64, we will delete the Compiler::lvaInitUserArgs{} ? IMO, I think we can uniform the classifier and the Compiler::lvaInitUserArgs{}, that is, can we cache the classifier info within the Compiler::lvaInitUserArgs{}

Yes, we will eventually remove the ABI code from lvaInitUserArgs once everything has moved. The function will stay and be responsible for creating and initializing LclVarDsc, but without any ABI related code.

There is no need to do the ABI classification at the same time. I looked at throughput when introducing the separation and the impact is minimal. Separating the concerns simplifies the code because we initialize parameters (like this pointer and ret buffer) from multiple places, not just lvaInitUserArgs.

Eventually my goal is to also use this for GenTreeCall ABI info.

@shushanhf
Copy link
Contributor

@jakobbotsch The new-style ABI is used to replace the Compiler::lvaInitUserArgs{} where resolve the args info, is it? If we add the classifier for LA64/RV64, we will delete the Compiler::lvaInitUserArgs{} ? IMO, I think we can uniform the classifier and the Compiler::lvaInitUserArgs{}, that is, can we cache the classifier info within the Compiler::lvaInitUserArgs{}

Yes, we will eventually remove the ABI code from lvaInitUserArgs once everything has moved. The function will stay and be responsible for creating and initializing LclVarDsc, but without any ABI related code.

There is no need to do the ABI classification at the same time. I looked at throughput when introducing the separation and the impact is minimal. Separating the concerns simplifies the code because we initialize parameters (like this pointer and ret buffer) from multiple places, not just lvaInitUserArgs.

Eventually my goal is to also use this for GenTreeCall ABI info.

OK, I see.
I will add the LoongArch64Classifier::Classify within the targetloongarch64.cpp.

@jakobbotsch
Copy link
Member Author

RISCV64 classifier was added in #101114 and LA64 classifier was added in #101224, so this issue can be closed.

LA64 also switched to the shared version of genHomeRegisterParams, but RISCV64 still has its own version. @tomeksowi do you plan to submit a PR to switch it?

@tomeksowi
Copy link
Contributor

but RISCV64 still has its own version. @tomeksowi do you plan to submit a PR to switch it?

Yes, I'm working on it

@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

No branches or pull requests

5 participants