[LoongArch64] coreclr-jit directory #62843
Conversation
Co-authored-by: Loongson's .NET-teams
ghost
added
the
community-contribution
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
|
|
@shushanhf Great to see this submitted! Could you please split this into one PR per part? It would be very hard to review and merge all parts together. |
Thanks for your suggest. |
I didn't know different PRs for each patch commiting well. Liking the #62885 , Is it Ok ? |
Yes, this PR is ok. I have updated the PR title to "[LoongArch64] coreclr-vm directory". Could you please use similar pattern for titles of the remaining LoongArch64 PRs? Also, could you please sign the Contributor License Agreement? |
Ok，Thanks. |
1 similar comment
Ok，Thanks. |
jkotas
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsAdd supporting for LoongArch64.
|
|
I had finished a new patch for this PR based on the newest main branch. |
Conflicts: src/coreclr/jit/codegencommon.cpp src/coreclr/jit/lower.cpp src/coreclr/jit/morph.cpp
|
I will update later for compiling errorrs after merging from the main. |
|
closing & reopening for triger CI |
I've been busy and/or sick the last few days. I plan to take a look at this PR again soon. Assuming you've addressed all the comments and feedback, and I don't see anything new of concern, I expect to merge this soon. |
Thanks. |
@SingleAccretion Thanks for taking the time to review this. Please focus on the common files; we're not worried too much about the LA-specific files. (Ideally any place where LA adds significant new code to common files is a sign that we should refactor things to reduce platform-specific code in common files, if possible.) |
Thanks very much for your review. |
| // For LoongArch64's ABI, maybe there is a padding. | ||
| // e.g. `struct {float a; long b;}` | ||
| offset += TARGET_POINTER_SIZE; |
There was a problem hiding this comment.
I am curious: will LA try to pass the following struct without padding in two registers?
[StructLayout(LayoutKind.Sequential, Pack = 4)]
struct MixedMultiRegStructNoPad
{
public int FltValue;
public double DblValue;
}There was a problem hiding this comment.
I think this will pad as each slot on the stack is eight bytes for this case.
There was a problem hiding this comment.
It would be useful to confirm the behavior in this case is what we want/expect.
Note there are many variations of this "custom layout" problem, e. g.:
[StructLayout(LayoutKind.Explicit, Size = 16)]
struct OddStruct
{
[FieldOffset(1)]
public int FltValue; // Misaligned
[FieldOffset(7)]
public double DblValue; // Also misaligned
}Where the native ABI doesn't necessarily cover it (I suppose I would expect the struct above to be passed in two integer registers as a union-like thing)
There was a problem hiding this comment.
I think there are some cases within the coreclr's test cases.
I will add a test case tomorrow.
There was a problem hiding this comment.
It takes some time to rebuild the test env based on the latest runtime before testing.
There was a problem hiding this comment.
It takes some time to rebuild the test env based on the latest runtime before testing.
Now I have to build a new SDK7 for LA but this usually is very difficut when building an new version SDK at the first time, because the latest installer usually can be built on the x64-linux.
Now the SDK6.0.2 is OK for LA and had tested the runtime's test-cases and ASP's cases and also some real projects tested by other companies.
If I can get a SDK7 liking the CI's testing env I can focus on testing and debuging the latest runtime on LA more efficiently.
| } | ||
|
|
||
| #if defined(TARGET_LOONGARCH64) | ||
| if (varTypeIsFloating(spillType) && emitter::isGeneralRegister(tree->GetRegNum())) |
There was a problem hiding this comment.
One way to interpret this code is to mean we now have LCL_VAR<V00 float> int nodes in the system.
Where do they come from?
There was a problem hiding this comment.
On LoongArch64's ABI, Float type arg is passed by integer register while there is no float register left.
There was a problem hiding this comment.
Yes, but that doesn't explain it fully.
For the caller (method being compiled), we would have LCL_VAR V00<float> float, i. e. both the local variable have TYP_FLOAT and the node. And obviously a TYP_FLOAT node cannot have an integer register assigned to it.
So is it the case for callees then? I. e. fgMorphArgs retyping?
Could you please share a JitDump where we will see this case?
There was a problem hiding this comment.
In test cases this is ok where LA's SDK6 is ok.
I will add some asm tomorrow.
|
/azp run runtime, runtime-coreclr superpmi-asmdiffs |
|
Azure Pipelines successfully started running 2 pipeline(s). |
when running hello-world within debug-mode.
|
Now I had tested the hello-world passed both the release and debug mode based on the latest main-LoongArch64. |
|
I am busying the publishing the SDK6.0-LoongArch64 for C# developers during the last two weeks. I had finished a runtime-env only for LoongArch64 which not the whole SDK7.0 for LA and had passed the hello-world within both the release and debug mode. |
There was a problem hiding this comment.
This is ready to be merged. @shushanhf Thanks for incorporating all the feedback. After this, I expect we will see more updates, in (much) smaller pull requests.
Thanks very much. |
Add supporting for LoongArch64.