-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add checks to prevent silent bad codegen from VN optimizations. #49504
Conversation
There are cases when two fields have the same name but different handles.
9a5e511
to
aca6170
Compare
@@ -7754,10 +7757,10 @@ void Compiler::fgValueNumberTree(GenTree* tree) | |||
rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet())); | |||
} | |||
|
|||
lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree); |
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.
there was one failure on a big method before this change, the case looked like this:
ASG(LCL_VAR V01, 0) <- def
ASG(LCL_FLD V01+4, 1) with NotAField <- both def and use
Read LCL_FLD V01+8 with LclFld <- use
and SSA created such numbers:
ASG(LCL_VAR V01, 0) <- lclDefSsaNum 0
ASG(LCL_FLD V01+4, 1) with NotAField <- lclDefSsaNum 1, use ssa num 0
Read LCL_FLD V01+8 with LclFld <- use ssa num 1
and when we did fgValueNumberTree
for Read LCL_FLD V01+8
we expected data ssa num 1
to be initialized but it was not because we used ssa num 0 for ASG(LCL_FLD V01+4, 1)
.
@sandreenko fields on generic types are handled specially (when using the runtime instead of crossgen2) The general rule is that instance fields are not actually created as unique FieldDesc, the runtime simply uses the fields on the matching canonical type. |
then can we support the old verifying functionality of runtime/src/coreclr/inc/corinfo.h Lines 2585 to 2595 in 2d0fe49
on all VMs and use it here, what would be the result if I call :
|
PTAL @briansull @BruceForstall @dotnet/jit-contrib that will probably be a discussion for some time rather than a ready-to-be-merged PR. |
Additional thoughts:
because
|
The current behavior of specifying the extra "owner" parameter to getFieldType is that it will be used to produce a more exact type handle. So... if you have the following class.
Assuming that you are working with a
And if And if So, if we want a function to verify that a given field is a member of a type, we'll need to add a new function. |
ping myself |
will finish later. |
VN optimizations are known to be a source of silent bad codegen issues, a good part of them happen because it works with maps [CORINFO_CLASS_HANDLE, CORINFO_FIELD_HANDLE] and when it allows us to access a class/struct with a field handle not from this class. Jit does not hit any asserts when it happens, as a result, we can have this:
The bug here is that
000114
's VN is $40, when it should be $180. The issue is that we accessV02
in000097
and000114
with different FieldHandles, both have names [X] but one is not from that struct type, in the past, it was a silent bad codegen, now it would be an assert when we try to set such FldSeq for a LclVar + dump now shows the actual handle values, so it is clearer from the dump what happens.Summary: our new contract for FldSeq on LclVar is that CORINFO_FIELD_HANDLE must belong to LclVar's CORINFO_CLASS_HANDLE, otherwise it has to be
NotAField()
. And each FldSeq list should dereference a field that exists in the previous class handle.And when we see similar issues but not with LclFld I expect we broader these checks to other nodes that have FldSeq.
There are small asm diffs:
the diffs are falling into two categories:
for example:
our
this
has typewhen we access it with a token for
in the past we were setting
CORINFO_FIELD_HANDLE
fromCanon
to LclVar withMethodSymbol
type, it could have led to errors if we accessed it usingCORINFO_FIELD_HANDLE
fromMethodSymbol
at the same time. Like:I believe we have enough deoptimizations in other places to be protected from such behavior, in this case, V08 is marked as address exposed so VN does not do optimizations with it.
getFieldClass
returs an unexpected value for some fields, I hope @davidwrighton could help me to understand such cases better:This change improves our consistency for LCL_FLD but leaves many things unchanged:
For example, when we work with raw addresses we set FldSeq without any checks, it allows us to have smaller diffs and raw addresses are not involved in the most dangerous optimizations but could be used for CSE, like:
and if we have
NotAFld
CSE can't match it with the sameADD
nodes, but if we have a realFldSeq
it could be done.And there are some questions that need to be addressed before the merge:
if (IsPseudoField())
is very optimistic, hope @briansull can tell me how they are used and how the check should be improved.comp->info.compCompHnd->getFieldClass
so often but we need to measure it to know for sure, probably we can saveCORINFO_CLASS_HANDLE
intoFieldSeqNode
, and then we would not need it but use a little extra memory.