Skip to content

JIT: Switch remainder of lowering to new ABI info #112544

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

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

jakobbotsch
Copy link
Member

This required introducing some new information in ABIPassingSegment for stack-passed segments about whether they own the full stack slot or not. Apple arm64 notoriously has cases where multiple parameters are packed in the same stack slot.

This required introducing some new information in `ABIPassingSegment`
for stack-passed segments about whether they own the full stack slot or
not. Apple arm64 notoriously has cases where multiple parameters are
packed in the same stack slot.
@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 Feb 14, 2025
Copy link
Contributor

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

//---------------------------------------------------------------
// CheckIsStruct: Verify that the struct ABI information is consistent with the IR node.
//
void CallArg::CheckIsStruct()
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to remove this; the backend is already be doing similar validation for IR shapes depending on the ABI information.

@jakobbotsch jakobbotsch marked this pull request as ready for review February 14, 2025 15:34
@jakobbotsch
Copy link
Member Author

jakobbotsch commented Feb 14, 2025

PTAL @dotnet/jit-contrib

No diffs. Some MinOpts TP regressions in the 0.1-0.2% range (and some larger outliers in collections with very few MinOpts contexts).

Detailed TP diff for MinOpts contexts only looks like:

Base: 198188647104, Diff: 198652714462, +0.2342%

503226159  : NA      : 49.19% : +0.2539% : private: struct GenTree * __cdecl Lowering::NewPutArg(struct GenTreeCall *, struct GenTree *, class CallArg *, enum var_types)
149634454  : NA      : 14.63% : +0.0755% : private: unsigned int __cdecl StackLevelSetter::PopArgumentsFromCall(struct GenTreeCall *)                                    
73920858   : +4.24%  : 7.23%  : +0.0373% : public: void __cdecl CallArgs::AddFinalArgsAndDetermineABIInfo(class Compiler *, struct GenTreeCall *)                        
10015964   : +2.21%  : 0.98%  : +0.0051% : protected: void __cdecl CodeGen::genCallPlaceRegArgs(struct GenTreeCall *)                                                    
6685913    : +8.72%  : 0.65%  : +0.0034% : public: void __cdecl Compiler::lvaClassifyParameterABI<class WinX64Classifier>(class WinX64Classifier &)                      
-22497456  : -2.32%  : 2.20%  : -0.0114% : private: void __cdecl StackLevelSetter::ProcessBlock(struct BasicBlock *)                                                     
-256862005 : -33.27% : 25.11% : -0.1296% : private: void __cdecl Lowering::LowerArg(struct GenTreeCall *, class CallArg *, bool)                                         

MSVC no longer inlines NewPutArg, so hopefully some of that will be resolved by PGO data. Also #112561 is paying off all of this cost and more (and will also make PopArgumentsFromCall completely disappear outside x86).

@jakobbotsch jakobbotsch requested a review from a team February 14, 2025 15:38
Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

//-----------------------------------------------------------------------------
// OnStackWithoutConsumingFullSlot:
// Create an ABIPassingSegment representing that a segment is passed on the
// stack, and which does not gets its own full stack slot.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// stack, and which does not gets its own full stack slot.
// stack, and which does not get its own full stack slot.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix this as part of a follow up

@jakobbotsch jakobbotsch merged commit b93a981 into dotnet:main Feb 14, 2025
120 checks passed
@jakobbotsch jakobbotsch deleted the lower-putarg-new-abi branch February 14, 2025 19:52
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants