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

Add initial MaxStack calculation #93244

Merged
merged 12 commits into from Oct 25, 2023
Merged

Add initial MaxStack calculation #93244

merged 12 commits into from Oct 25, 2023

Conversation

buyaa-n
Copy link
Member

@buyaa-n buyaa-n commented Oct 9, 2023

This is initial MaxStack calculation for simple (no branching) instructions, basically UpdateStackSize(OpCode opCode) will be called for each OpCode emit and updates the _currentStack based on its StackBehaviourPush, StackBehaviourPop values.

        private void UpdateStackSize(OpCode opCode)
        {
            _currentStack += GetStackChangeFor(opCode.StackBehaviourPush) + GetStackChangeFor(opCode.StackBehaviourPop);

            if (_currentStack > _maxStackSize)
            {
                _maxStackSize = _currentStack;
            }
        }

This calculation will be updated as needed when branching IL.Emit overloads implemented.

As I cannot use the internal OpCode.StackChange() method created int GetStackChangeFor(StackBehaviour stackBehaviour) method

Contributes to #92975
Fixes #93419

@ghost
Copy link

ghost commented Oct 9, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Issue Details

This is initial MaxStack calculation for simple (no branching) instructions, basically UpdateStackSize(OpCode opCode) will be called for each OpCode emit and updates the _currentStack based on its StackBehaviourPush, StackBehaviourPop values

        private void UpdateStackSize(OpCode opCode)
        {
            _currentStack += GetStackChangeFor(opCode.StackBehaviourPush) + GetStackChangeFor(opCode.StackBehaviourPop);

            if (_currentStack > _maxStackSize)
            {
                _maxStackSize = _currentStack;
            }
        }

This calculation will be updated as needed when branching IL.Emit overloads implemented.

Contributes to #92975

Author: buyaa-n
Assignees: -
Labels:

area-System.Reflection.Emit

Milestone: -

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
…t/ILGeneratorImpl.cs

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM pending API approval.

@steveharter
Copy link
Member

Note there are some unaccounted test failures. Perhaps rebase\merge to latest since I see some recent JIT fixes went in.

@buyaa-n buyaa-n requested a review from jkotas October 24, 2023 21:43
@jkotas
Copy link
Member

jkotas commented Oct 24, 2023

Add test for the new API under https://github.com/dotnet/runtime/blob/fe043b489f69a888ad2efd97fbc177c1f25e541d/src/libraries/System.Reflection.Emit/tests/OpCodesTests.cs

@buyaa-n
Copy link
Member Author

buyaa-n commented Oct 25, 2023

Failures unrelated and already reported in #88102, #88027

@buyaa-n buyaa-n merged commit 6794c36 into dotnet:main Oct 25, 2023
171 of 175 checks passed
@buyaa-n buyaa-n deleted the il_stack branch October 25, 2023 19:00
liveans pushed a commit to liveans/dotnet-runtime that referenced this pull request Nov 9, 2023
* Add initial MaxStack calculation and tests

* Apply suggestions from code review

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

* Make OpCode.StackChange() public and use the method

* Apply the approved API shape

* Add doc, update the ref API ordering

* Add more doc info, add test

* Update doc remarks

---------

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Make OpCode.StackChange() public
5 participants