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

[NativeAOT-LLVM] Add tracked SSA locals to the first basic block LiveIn set. #2684

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

yowl
Copy link
Contributor

@yowl yowl commented Sep 11, 2024

This PR adds tracked SSA variables to the Live In set for the first basic block, fixing an assert in release config.

Fixes #2682

@yowl yowl marked this pull request as ready for review September 11, 2024 21:46
@yowl
Copy link
Contributor Author

yowl commented Sep 11, 2024

cc @dotnet/nativeaot-llvm

Thanks @SingleAccretion for the fix.

Copy link

@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.

Could you also add a test for this case?

src/coreclr/jit/llvmlssa.cpp Outdated Show resolved Hide resolved
@yowl
Copy link
Contributor Author

yowl commented Sep 17, 2024

Could you also add a test for this case?

Any idea what that would look like? I tried a few things, but not sure exactly what triggered the error.

@SingleAccretion
Copy link

Any idea what that would look like? I tried a few things, but not sure exactly what triggered the error.

It should cause the initial and unused definition of a tracked parameter to become live due to a safe point between it and the first (actual) definition. So, something like:

object func(object a) {
   SafePoint();
   a = new object();
   SafePoint(); // Force 'a' to the shadow stack
   return a;
}

@yowl
Copy link
Contributor Author

yowl commented Sep 17, 2024

Thanks, that does it.

yowl and others added 2 commits September 17, 2024 09:58
Copy link

@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.

LGTM. Thank you!

@pavelsavara pavelsavara merged commit 4ece14b into dotnet:feature/NativeAOT-LLVM Sep 18, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants