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

[LoongArch64] Fix the wrong GCInfo when testing GCStress. #72572

Merged
merged 13 commits into from
Aug 4, 2022

Conversation

shushanhf
Copy link
Contributor

This is part of the issue #69705 to amend the LA's port.

This PR fixs the wrong GCInfo when testing GCStress, adds split for LA and fixs some assert errors.

@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 Jul 21, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 21, 2022
@ghost
Copy link

ghost commented Jul 21, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

This is part of the issue #69705 to amend the LA's port.

This PR fixs the wrong GCInfo when testing GCStress, adds split for LA and fixs some assert errors.

Author: shushanhf
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

src/coreclr/jit/jit.h Outdated Show resolved Hide resolved
src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
Comment on lines 3939 to 3942
#ifndef TARGET_LOONGARCH64
// Make sure we loaded exactly the required amount of bytes.
// But for LoongArch64's ABI, the struct {long a; float b;} may be passed
// by Integer register and float register while must be considered the padding here.
assert(structSize == (lastElem.Offset + genTypeSize(lastElem.Type)));
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not right, this path must always load the exact size, it means the logic for setting up elems is not correct.

What does the elems array look like for the cited struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

structSize=16; for struct {long a; float b;}

Copy link
Contributor Author

@shushanhf shushanhf Jul 21, 2022

Choose a reason for hiding this comment

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

3941	            assert(structSize == (lastElem.Offset + genTypeSize(lastElem.Type)));
(gdb) p structSize
$1 = 16
(gdb) p lastElem
$2 = (Compiler::ArgElem &) @0xfffffee970: {Type = TYP_FLOAT, Offset = 8}
(gdb) p elems[0]
$3 = {Type = TYP_LONG, Offset = 0}
(gdb) p elems[1]
$4 = {Type = TYP_FLOAT, Offset = 8}

Copy link
Contributor

@SingleAccretion SingleAccretion Jul 21, 2022

Choose a reason for hiding this comment

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

And the second element is 4 bytes in size, I see.

Only some subset of structs can pass 4 bytes only instead of the full 8 (i. e. with the padding). For example, with this struct:

[StructLayout(LayoutKind.Sequential, Size = 16)]
struct StructWithSignificantPadding
{
    long L;
    float F;
    [padding: 4]
}

All 16 bytes must be passed (we assume the user could store something in the explicit padding).

It seems simpler to always pass all the bytes (this would also be consistent with Unix x64).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks very much for your reviewing and advices!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks very much.
I will reply later, maybe tomorrow night.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only some subset of structs can pass 4 bytes only instead of the full 8 (i. e. with the padding). For example, with this struct:

[StructLayout(LayoutKind.Sequential, Size = 16)]
struct StructWithSignificantPadding
{
    long L;
    float F;
    [padding: 4]
}

All 16 bytes must be passed (we assume the user could store something in the explicit padding).

Or the struct

[StructLayout(LayoutKind.Sequential, Size = 16)]
struct StructWithSignificantPadding
{
    double d;
    int i;
    [padding: 4]
}
  • (1) all 16 bytes mus be passed.
    For the CPUs liking the LA64 and MIPS64, the struct passed to the callee function, if the struct's field within the register which including the padding does't be stored on the stack within the prolog and directly as an operand for a operation, liking this,
### the register A1 containg the second field with padding
IG_01:
       ###prolog, and A1 not be stored to stack
IG_02:
       ###the 32bits operation liking  add_w, compare_w and branch:
       addi.w    T0, A1, 0x4        ### the hight 32bits of the A1 must be sign extend, or the result is unpredictable.
                                               ### This is quite different with the AArch64 and AMD64.
  • (2) I want to know the padding will be used at this case ?
    For example, the register which including the padding will be passed to an instruction directly liking the previous example ?
  • (3) I want more detail info about the usage of the padding to amend the LA64's ABI implementation,
    for example, adding more details info about the padding and specially treat it before using it directly, maybe store the padding first and then sign extend the hight 32bits.

Copy link
Contributor

Choose a reason for hiding this comment

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

So essentially the question is how should the "custom ABI" for these structs look like. Not a simple problem.

One thing to consider is that we must retain compatibility with the native ABI for C/C++ PInvokes.

For example, the following struct:

[StructLayout(LayoutKind.Explicit)]
struct S
{
    [FieldOffset(0)]
    public long U1;
    [FieldOffset(0)]
    public long U2;
    [FieldOffset(8)]
    public int I;
}

Equivalent to this native one:

struct S
{
    union
    {
        long long U1;
        long long U2;
    }
    int I;
}

Would be classified as having "significant padding" in the managed world. So it would have to be passed one way to managed methods and another way to native ones.

For the CPUs liking the LA64 and MIPS64, the struct passed to the callee function, if the struct's field within the register which including the padding doesn't be stored on the stack

Structs with significant padding will not be promoted, and so the situation you cite ought not to arise (in the Jitted code at least).

The general question does still stand on how the padding should be treated though (with a "it is sequence of byte fields"-like rule), so we can reason about it.

It looks like a significant decision to make, so I will ask @jkotas and @dotnet/jit-contrib for assistance here.

My personal thinking here is that we should make this as simple as possible, perhaps going as far as passing all structs with significant padding by reference. I suppose this also requires fixing #71711 first.

Copy link
Member

Choose a reason for hiding this comment

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

I do not have an opinion on this one. LA64 ABI for passing structs is extremely complex. I do not have all details internalized.

One thing to consider is that we must retain compatibility with the native ABI for C/C++ PInvokes.
For example, the following struct:

Yes, native ABI compatibility is a good thing to shoot for. However, we do not have 100% interop for unions today. I believe that there are cases on existing platforms where it is not possible to define a struct with unions in a portable way in C#.

Copy link
Contributor Author

@shushanhf shushanhf Jul 23, 2022

Choose a reason for hiding this comment

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

Yes, native ABI compatibility is a good thing to shoot for.

Agree with you.

However, we do not have 100% interop for unions today. I believe that there are cases on existing platforms where it is not possible to define a struct with unions in a portable way in C#.

Yes, I think so that defining a struct with union is not a good way for keeping portable.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Thanks @SingleAccretion for your review. Added few more things.

src/coreclr/jit/gentree.cpp Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/emitloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/emitloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/emitloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/emitloongarch64.cpp Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jul 25, 2022
@shushanhf shushanhf force-pushed the LA_GCInfo branch 2 times, most recently from d2dcfd6 to 1e3252d Compare July 28, 2022 02:36
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Aug 1, 2022
@shushanhf
Copy link
Contributor Author

Hi, @kunalspathak @SingleAccretion
What should I do next ?

@hez2010
Copy link
Contributor

hez2010 commented Aug 2, 2022

What should I do next ?

It's very close to the net7.0 branch freezing. This PR may be held until the main branch is updated to net8.0.

Copy link
Contributor

@SingleAccretion SingleAccretion left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I don't think we should block this change on resolving the issues with "structs with significant padding".

@kunalspathak
Copy link
Member

on resolving the issues with "structs with significant padding".

This is for loongarch only. @shushanhf - Is the loongarch port tied with .NET 7 release somehow or can we wait to take this in .NET 8?

@shushanhf
Copy link
Contributor Author

on resolving the issues with "structs with significant padding".

This is for loongarch only. @shushanhf - Is the loongarch port tied with .NET 7 release somehow or can we wait to take this in .NET 8?

I think it's better to merge this within .NET7 and amending it after struct's padding is explicit.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Added some questions/comments.

src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Show resolved Hide resolved
src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Aug 2, 2022
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

Some comments are still unclear and we need to fix that up before we merge.

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/codegenloongarch64.cpp Outdated Show resolved Hide resolved
@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Aug 3, 2022
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@kunalspathak kunalspathak merged commit 76533d4 into dotnet:main Aug 4, 2022
@shushanhf shushanhf deleted the LA_GCInfo branch August 5, 2022 00:45
@ghost ghost locked as resolved and limited conversation to collaborators Sep 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-loongarch64 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants