Skip to content

JIT: generalize ParseArrayAddress a bit more #112527

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 2 commits into from
Feb 13, 2025

Conversation

AndyAyersMS
Copy link
Member

Now that we are stack allocating arrays, the array reference may be either TYP_BYREF or TYP_I_IMPL, so try and recognize those cases when possible. In particular for OSR methods the array allocation may "disappear" once we redirect flow to the OSR entry, so relying on VN is insufficient.

Fixes #112429 (benchmark regression where we're measuring OSR code)

Now that we are stack allocating arrays, the array reference may be either TYP_BYREF
or TYP_I_IMPL, so try and recognize those cases when possible. In particular for OSR
methods the array allocation may "disappear" once we redirect flow to the OSR entry,
so relying on VN is insufficient.

Fixes dotnet#112429 (benchmark regression where we're measuring OSR code)
@Copilot Copilot AI review requested due to automatic review settings February 13, 2025 15:59
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • src/coreclr/jit/gentree.cpp: Language not supported

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 13, 2025
@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

Would be nice not to have to have ParseArrayAddress at all. We could just keep the array address components distinct until later, but unfortunately VN/Hoisting/CSE are very literal minded and we'd lose some opts. That is if ARR_ADD was a 3-term add, and two of the three terms were say loop invariant or common, we'd have no way of doing a hoist / CSE.

Comment on lines 19904 to 19905
const unsigned lclNum = tree->AsLclVar()->GetLclNum();
CORINFO_CLASS_HANDLE hnd = comp->lvaTable[lclNum].lvClassHnd;
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
const unsigned lclNum = tree->AsLclVar()->GetLclNum();
CORINFO_CLASS_HANDLE hnd = comp->lvaTable[lclNum].lvClassHnd;
CORINFO_CLASS_HANDLE hnd = comp->lvaGetDesc(tree->AsLclVar())->lvClassHnd;

Copy link
Member

Choose a reason for hiding this comment

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

(mostly just want to see lvaGetDesc instead of raw access into lvaTable)

@AndyAyersMS AndyAyersMS merged commit abe6b9e into dotnet:main Feb 13, 2025
111 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 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.

[Perf] Linux/x64: Benchstone.BenchF.SqMtx Regression on 2/9/2025 2:06:49 AM +00:00
2 participants