-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
JIT: Fold string fields in structs stored in static readonly fields #80431
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue Detailsstruct MyStruct
{
public string Name;
}
// static readonly field holding a struct
static readonly MyStruct MyStr = new() { Name = "Hey!" };
static string GetName() => MyStr.Name; // optimized to just "Hey!" Codegen for 48B8C01E8057D9020000 mov rax, 0x2D957801EC0
488B00 mov rax, gword ptr [rax]
488B4008 mov rax, gword ptr [rax+08H]
C3 ret
; Total bytes of code 18 after: 48B8B884DAABFD7F0000 mov rax, 0x7FFDABDA84B8 ; 'Hey!'
C3 ret
; Total bytes of code 11,
|
Aside from the actual PR and work: Big thanks @EgorBo for the way you write the PR descriptions that show an example, before and after codegen. |
@jkotas could you please take a look at the VM side? I had to modify the API to scan structs for GC fields |
The PR doesn't produce lots of diffs but it's currently blocked on #78736 e.g. static OSPlatform OS { get; } = new OSPlatform("Windows");
static string Test() => OS.Name;
|
src/tests/JIT/opt/ValueNumbering/StaticReadonlyStructWithGC.csproj
Outdated
Show resolved
Hide resolved
src/coreclr/jit/valuenum.cpp
Outdated
if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && | ||
// it should be a static field | ||
(fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && | ||
// boxed static to be precise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fragile condition. We are assuming that the runtime uses boxed statics for this pattern. Is there a way to write this in a more general way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runtime/src/coreclr/jit/valuenum.cpp
Lines 5125 to 5145 in c0b6956
void Compiler::fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, FieldSeq* fieldSeq, ssize_t offset) | |
{ | |
noway_assert(fieldSeq != nullptr); | |
// Check if the load represents a frozen gc object located in a struct which is stored in a static readonly field, | |
// e.g.: | |
// | |
// struct MyStruct { string Name; } | |
// static readonly MyStruct MyStr = new() { Name = "Hey!" }; | |
// | |
// string GetName() => MyStr.Name; // <- loadTree | |
// | |
if ((baseAddr == nullptr) && loadTree->TypeIs(TYP_REF) && | |
// it should be a static field | |
(fieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStatic) && | |
// boxed static to be precise | |
(fieldSeq->GetOffset() == TARGET_POINTER_SIZE)) | |
{ | |
uint8_t buffer[TARGET_POINTER_SIZE] = {0}; | |
if (((UINT)offset < INT_MAX) && | |
info.compCompHnd->getReadonlyStaticFieldValue(fieldSeq->GetFieldHandle(), buffer, TARGET_POINTER_SIZE, |
we accept loadTree that looks like this:
and the fieldseq that represents that load. So technically I don't need to check that tree at all. @SingleAccretion @jakobbotsch could you please validate JIT part of this PR?
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
…nly-struct-string
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…runtime-1 into vn-static-readonly-struct-string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why is this not done in fgValueNumberConstLoad
like for other types.
src/coreclr/jit/valuenum.cpp
Outdated
@@ -9965,6 +9965,19 @@ void Compiler::fgValueNumberSsaVarDef(GenTreeLclVarCommon* lcl) | |||
// | |||
static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, ssize_t* byteOffset, FieldSeq** pFseq) | |||
{ | |||
VNFuncApp funcApp; | |||
if (tree->TypeIs(TYP_REF) && vnStore->GetVNFunc(tree->gtVNPair.GetLiberal(), &funcApp) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be TYP_BYREF
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point in checking the type of the address?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any point in checking the type of the address?
no, removed 🙂
good point, addressed. jit-diff is around -5kb. Overall, it was an exercise to fold |
@jkotas could you please review the VM side? NativeAOT side is not implemented because of #83043 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Codegen for
GetName()
before:after: