Skip to content

Optimize casts from constant byrefs #99130

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+8 −0
Open

Optimize casts from constant byrefs #99130

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad35f3a
Optimize casts from constant byrefs
MichalPetryka Feb 29, 2024
e52a42b
Retype BYREF constants instead of folding a new cast
MichalPetryka Mar 2, 2024
d234bef
Merge branch 'main' into byref-cast
MichalPetryka Mar 22, 2024
002f7ef
Merge branch 'main' into byref-cast
MichalPetryka Jun 14, 2024
c2fca8f
Merge branch 'main' into byref-cast
MichalPetryka Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,13 @@ GenTree* Compiler::fgMorphExpandCast(GenTreeCast* tree)
// we fix this by copying the GC pointer to a non-gc pointer temp.
noway_assert(!varTypeIsGC(dstType) && "How can we have a cast to a GCRef here?");

// we can just change the type for constant addresses
if (oper->IsCnsIntOrI())
{
oper->gtType = TYP_I_IMPL;
return fgMorphTree(gtNewCastNode(TYP_I_IMPL, oper, false, dstType));
Copy link
Member

@EgorBo EgorBo Feb 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This might create e.g. a nongc icon handle typed as TYP_I_IMPL. I think we have a couple of asserts expecting those to be always TYP_REF. What exactly this change improves?

Copy link
Contributor Author

@MichalPetryka MichalPetryka Mar 1, 2024

Choose a reason for hiding this comment

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

  1. The cast here seems to strip handle-ness.
  2. Would restricting it to TYP_BYREF make it okay?
  3. Added codegen diff to the PR description.

Copy link
Member

@EgorBo EgorBo Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. The cast here seems to strip handle-ness.

Which doesn't sound right e.g. - does it strip it for relocable (AOT) handles as well?

  1. Would restricting it to TYP_BYREF make it okay?
  2. Added codegen diff to the PR description.

Diffs are small and are regressions so does it make sense?

Copy link
Contributor Author

@MichalPetryka MichalPetryka Mar 2, 2024

Choose a reason for hiding this comment

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

Which doesn't sound right e.g. - does it strip it for relocable (AOT) handles as well?

Made it retype the handles instead.

Diffs are small and are regressions so does it make sense?

The only diff currently is from a GC hole that's fixed in #99138, I'd guess this won't have any diffs in the BCL or tests outside of #99011 since those don't generally use AsPointer on statics or RVA statics.

Copy link
Member

@EgorBo EgorBo Mar 2, 2024

Choose a reason for hiding this comment

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

It's nice that it helped you to find a gc hole! No diff in BCL means it's basically untested, it's probably fine now but may fail in future if we introduce byref icon handle? I think we should do changes like these only if they provide clear benefits and risks are understood

}

// We generate an assignment to an int and then do the cast from an int. With this we avoid
// the gc problem and we allow casts to bytes, longs, etc...
unsigned lclNum = lvaGrabTemp(true DEBUGARG("Cast away GC"));
Expand Down