-
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: Change GTF_ICON_INITCLASS -> GTF_IND_INITCLASS #85396
Conversation
This reverts commit 3699de5.
This flag represents that dereferences off of the handle are dependent on a cctor and cannot be hoisted out unless all cctors also are. It must be propagated together with the constant for correctness.
This reverts commit c6a1fca.
The JIT has a flag GTF_ICON_INITCLASS that represents that accesses off that address are cctor dependent. Hoisting uses this to avoid hoisting cctor dependent indirections unless all cctors are also hoisted. However, local constant prop/VN-based constant prop do not handle this flag, so we could run into cases where addresses with GTF_ICON_INITCLASS were propagated and then subsequently hoisted incorrectly. This change moves the flag to an OperIsIndir() flag instead of being a flag on the constant. After some digging, I found that the original reason the flag was not an indir flag was simply that there were no more indir flags available, but we do have available flags today. This fix is much simpler than the alternatives which would be to teach VN/local copy prop to propagate this GTF_ICON_INITCLASS flag. Also remove GTF_FLD_INITCLASS which is never set.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThe JIT has a flag GTF_ICON_INITCLASS that represents that accesses off This change moves the flag to an OperIsIndir() flag instead of being a Also remove GTF_FLD_INITCLASS which is never set. Fixes a problem I hit in #85323.
|
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, surprised that didn't lead to problems previously
The JIT has a flag GTF_ICON_INITCLASS that represents that accesses off
that address are cctor dependent. Hoisting uses this to avoid hoisting
cctor dependent indirections unless all cctors are also hoisted.
However, local constant prop/VN-based constant prop do not handle this
flag, so we could run into cases where addresses with GTF_ICON_INITCLASS
were propagated and then subsequently hoisted incorrectly.
This change moves the flag to an OperIsIndir() flag instead of being a
flag on the constant. After some digging I found that the original
reason the flag was not an indir flag was simply that there were no more
indir flags available, but we do have available flags today. This fix
is much simpler than the alternatives which would be to teach VN/local
copy prop to propagate this GTF_ICON_INITCLASS flag.
Also remove GTF_FLD_INITCLASS which is never set.
Fixes a problem I hit in #85323.
FWIW, I don't think the problem mentioned back then to warrant being conservative and putting it on the constant exists: if a
ldsflda
feeds an indirection then the indirection is not going to be a candidate for hoisting regardless (unless something marks it invariant based on some analysis, in which case we can also mark it withGTF_IND_INITCLASS
there -- but let's cross that bridge if we get to it).