-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[LoongArch64] JIT/EE interface for getting ABI-info #62893
Conversation
update from runtime.
…-info. (#59561) Co-authored-by: Loongson's .NET-teams
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsadd ToolBox directory about jitinterace for getting ABI-info.
|
Could you please add links to the LoongArch64 calling convention spec to https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md as part of this PR? |
OK. |
Is there a spec that describes passing of non-primitive types (e..g struct with multiple fields that the new JIT/EE interface methods are dealing with)? https://github.com/loongson/LoongArch-Documentation/blob/main/docs/LoongArch-ELF-ABI-EN.adoc does not seem to be cover it. |
Sorry for late reponse. Later, I will find or ask where is the full version of the english ABI-spec. Thanks~ |
According to https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf, the rules for RISC-V are:
If the rules are as simple as this, then I think the JIT/EE interface change can be simplified a lot. I think it can be just a single flag that says whether the struct is passed in floating point registers. It should be possible to infer the rest through existing APIs. Thoughts? |
yes, but there are a lot of details. |
src/coreclr/vm/jitinterface.cpp
Outdated
} | ||
else if (fieldType == ELEMENT_TYPE_VALUETYPE) | ||
{ | ||
pMethodTable = pFieldStart->LookupApproxFieldTypeHandle().GetMethodTable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LookupApproxFieldTypeHandle can return null. Is there anything that guarantees it won't return null in this case?
Should this rather use GetFieldTypeHandleThrowing
that will load the types as necessary and never return null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
You are right.
In fact, we had updated it by using a if (pMethodTable)
and it works ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, we had updated it by using a if (pMethodTable) and it works ok.
That's not a correct fix. LookupApproxFieldTypeHandle
will return null when the type was not loaded before. Using LookupApproxFieldTypeHandle
will lead to non-deterministic behavior - the behavior will depend on whether something else in the program happened to load the type or not.
You should change this code to call GetFieldTypeHandleThrowing
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, we had updated it by using a if (pMethodTable) and it works ok.
Yes, this is only a workround way.
That's not a correct fix.
LookupApproxFieldTypeHandle
will return null when the type was not loaded before. UsingLookupApproxFieldTypeHandle
will lead to non-deterministic behavior - the behavior will depend on whether something else in the program happened to load the type or not.You should change this code to call
GetFieldTypeHandleThrowing
.
Thanks a lot !!!
I will update it.
The general shape looks fine to me. I have left some additional comments. |
Hi, @jkotas Should I create a new PR for adding |
The implementation should be part of this PR. |
OK, I will upload it later. |
by `GetFieldTypeHandleThrowing()`.
src/coreclr/tools/Common/JitInterface/LoongArch64PassStructInRegister.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The runtime and SDK for LoongArch64 are based on the 6.0.102 while the sdk-LoongArch64 was made by ourself.
src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunReader.cs
Outdated
Show resolved
Hide resolved
This reverts commit b05a2b9. The crossgen2 for LoongArch64 will be submitted by a new PR.
@shushanhf Are there are changes you plan to make in this PR? We're waiting for this one to be complete, and be merged, before reviewing #62885 |
Hi, @BruceForstall Thanks.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple minor comments.
Plus: you need to change the JIT/EE version in https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/jiteeversionguid.h, since you've changed the interface.
@jkotas Do you have any final comments/requests before this gets merged? |
@BruceForstall |
src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp
Outdated
Show resolved
Hide resolved
Added a few nits. LGTM otherwise. |
Also delete some unused comments.
Conflicts: src/coreclr/inc/jiteeversionguid.h
Hmmm, probably not, but it looks like you figured out how to get a new GUID (or just chose one randomly). |
add ToolBox directory about jitinterace for getting ABI-info.