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

Cleanup LclVar alignment requirements. #46026

Open
sandreenko opened this issue Dec 14, 2020 · 2 comments
Open

Cleanup LclVar alignment requirements. #46026

sandreenko opened this issue Dec 14, 2020 · 2 comments
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI design-discussion Ongoing discussion about design without consensus
Milestone

Comments

@sandreenko
Copy link
Contributor

sandreenko commented Dec 14, 2020

The current alignment mechanism for jit local vars is very complex, I think it is worth a cleanup.

Requirements:

  1. There are different requirements for incoming arguments and local vars in the method:
    1.1 the incoming argument requirements are simple, esp. without vector calling convention.
  2. There are correctness and performance requirements for local vars:
    2.1 there are more correctness requirements for armArch, less for XARCH;

I propose to create 2 functions:

  • getAlignmentForArg();
  • getAlignmentForLocal(bool correctness/performance);

What do we have now:

  1. There is genTypeAlignments table that is defined in "typelist.h" and is used only in InferOpSizeAlign for ArmArch, for arm64 it is used only in a stress mode, for arm32 it is used in fgInitArgInfo;

  2. How do we get alignment for arm64? It happens in lvaAllocLocalAndSetVirtualOffset, but this function has correct code only for 64-bit target, for arm32 we set the correct alignment before calling it:

    /* Need to align the offset? */

    and then call the function that handles 64-bit"
    stkOffs = lvaAllocLocalAndSetVirtualOffset(lclNum, lvaLclSize(lclNum), stkOffs);

the correctness requirements are hardcoded into the code, they are not coming from "typelist.h".

  1. for arguments we usually just set alignment to TARGET_POINTER_SIZE and it works as long as we don't have vector calling convention, if we get it we will have the wrong alignment for arm/arm64.

With the new requirements for arm64 apple I had to check and change all of these places for the new ABI.

I suggest to delete alignment information from "typelist.h", delete genTypeAlignments, replace code in lvaAllocLocalAndSetVirtualOffset and lvaAssignVirtualFrameOffsetsToLocals with a call to getAlignmentForLocal;

Stress code in lvaStressLclFldCB also should be changed to use getAlignmentForLocal, the old code that was working only for armArch should be deleted.

category:design
theme:alignment
skill-level:intermediate
cost:medium
impact:small

@sandreenko sandreenko added design-discussion Ongoing discussion about design without consensus area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Dec 14, 2020
@sandreenko sandreenko added this to the 6.0.0 milestone Dec 14, 2020
@sandreenko sandreenko self-assigned this Dec 14, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Dec 14, 2020
@sandreenko
Copy link
Contributor Author

@AndyAyersMS I would like to get your opinion about it if you think it is worth doing now.

@AndyAyersMS
Copy link
Member

Generally I'm in favor of things that remove complexity (or at least try and limit the scope of complexity). This proposal seems like it is likely to do just that. So it seems like a good idea.

@sandreenko sandreenko removed the untriaged New issue has not been triaged by the area owner label Dec 14, 2020
@JulieLeeMSFT JulieLeeMSFT added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 23, 2021
@briansull briansull removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Apr 8, 2021
@sandreenko sandreenko modified the milestones: 6.0.0, Future Jun 3, 2021
@JulieLeeMSFT JulieLeeMSFT moved this to Backlog (General) in .NET Core CodeGen Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI design-discussion Ongoing discussion about design without consensus
Projects
Status: Backlog (General)
Development

No branches or pull requests

5 participants