Repository navigation
Allow TYP_STRUCT LCL_FLD on the LHS of block copies - #70782
Conversation
ghost
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFor now, we'll do the simple thing and lower it as STORE_OBJ/BLK, completing No diffs are expected.
|
fc29143 to
676f8ed
Compare
For now, we'll do the simple thing and lower it as STORE_OBJ/BLK. Almost no diffs.
676f8ed to
5bb4b57
Compare
|
@dotnet/jit-contrib |
| { | ||
| assert(!tree->OperIsLocalField() && "do not expect struct local fields."); | ||
| lclVarType = GetLayout()->GetRegisterType(); | ||
| layout = tree->AsLclFld()->GetLayout(); |
There was a problem hiding this comment.
The documentation on this function seems a little misleading now. Generally it feels like the function should maybe be renamed something like GetRegisterTypeForAccess?
There was a problem hiding this comment.
The documentation on this function seems a little misleading now.
Do you refer to the "local var" part?
//------------------------------------------------------------------------
// GetRegisterType: Determine register type for this local var. <-- should be "local node"
//
// Arguments:
// tree - node that uses the local, its type is checked first.
//
// Return Value:
// TYP_UNDEF if the layout is not enregistrable, the register type otherwise.
//
(The function handled local fields before too, just now we have those struct-typed sometimes as well.)
I don't have an opinion on the name; I can rename it if you think it'd be clearer.
There was a problem hiding this comment.
Yes. I guess most of all it seems like a weird inversion to me that this is a function on LclVarDsc when the local is completely determined by the local node.
Let's leave the potential cleanup to a future change since this handled primitively typed LCL_FLD nodes before as well.
There was a problem hiding this comment.
I guess most of all it seems like a weird inversion to me that this is a function on LclVarDsc when the local is completely determined by the local node.
Ah yes, this is because we don't have access to the LclVarDsc* from inside GenTree*. The more idiomatic pattern for this problem in the code is to have GenTree::NeedsLclVarDsc(Compiler* compiler), here it was put on LclVarDsc instead, I suppose because it was already available at most callsites.
Let's leave the potential cleanup to a future change since this handled primitively typed LCL_FLD nodes before as well.
Added to my TODO list.
For now, we'll do the simple thing and lower it as STORE_OBJ/BLK, completing
TYP_STRUCTLCL_FLDsupport for assignments.The diffs are minimal and consist of one regression where the GS security logic marks one more parameter as needing a shadow copy (the GS analysis logic treats
ASG(LCL_FLD, ...)differently fromASG(OBJ(ADDR(LCL)), ...)), and a couple zero-sized diffs from the same GS logic reordering the stack.