Uh oh!
There was an error while loading. Please reload this page.
[LoongArch64] JIT/EE interface for getting ABI-info - #62893
Conversation
update from runtime.
…-info. (#59561) Co-authored-by: Loongson's .NET-teams
ghost
commented
Dec 16, 2021
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsadd ToolBox directory about jitinterace for getting ABI-info.
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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? |
shushanhf
commented
Dec 17, 2021
OK. |
jkotas
commented
Dec 17, 2021
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. |
shushanhf
commented
Dec 18, 2021
Sorry for late reponse. Later, I will find or ask where is the full version of the english ABI-spec. Thanks~ |
jkotas
commented
Dec 18, 2021
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. |
| // bit 5: `1` means the second field's size is 8. | ||
| // | ||
| // Note that bit 0 and 3 cannot both be set. | ||
| enum StructFloatFieldInfoFlags : uint8_t |
There was a problem hiding this comment.
This does not need to be uint8_t. The default enum underlying type should work just fine.
There was a problem hiding this comment.
OK, I will update it by dnderlying type.
Thanks.
| } | ||
| else if (fieldType == ELEMENT_TYPE_VALUETYPE) | ||
| { | ||
| pMethodTable = pFieldStart->LookupApproxFieldTypeHandle().GetMethodTable(); |
There was a problem hiding this comment.
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.
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.
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.
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.
LookupApproxFieldTypeHandlewill return null when the type was not loaded before. UsingLookupApproxFieldTypeHandlewill 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.
Uh oh!
There was an error while loading. Please reload this page.
jkotas
commented
Feb 14, 2022
The general shape looks fine to me. I have left some additional comments. |
shushanhf
commented
Feb 15, 2022
Hi, @jkotas Should I create a new PR for adding |
jkotas
commented
Feb 15, 2022
The implementation should be part of this PR. |
shushanhf
commented
Feb 15, 2022
OK, I will upload it later. |
by `GetFieldTypeHandleThrowing()`.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This reverts commit b05a2b9. The crossgen2 for LoongArch64 will be submitted by a new PR.
BruceForstall
commented
Feb 17, 2022
@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.
|
BruceForstall
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
BruceForstall
commented
Feb 18, 2022
@jkotas Do you have any final comments/requests before this gets merged? |
shushanhf
commented
Feb 18, 2022
@BruceForstall |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
jkotas
commented
Feb 18, 2022
Added a few nits. LGTM otherwise. |
Also delete some unused comments.
Conflicts: src/coreclr/inc/jiteeversionguid.h
BruceForstall
commented
Feb 18, 2022
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.