-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Do not create non-null assertions from location nodes #62743
Do not create non-null assertions from location nodes #62743
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsSome indirection nodes in the compiler represent locations, not values. These are either the LHSs of The problem is that we also cannot detect them reliably. The optimization phases use the Thus, the solution I propose is a conservative one, but correct: rely on Contributes to #13762. We are expecting mixed diffs: the change unblocks some CSEs (because their defs now have the exception sets of their uses).
|
1087671
to
2610c1e
Compare
@dotnet/jit-contrib |
2610c1e
to
1cb26f9
Compare
1cb26f9
to
2f8d248
Compare
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, thanks for the fix!
Some indirection nodes in the compiler represent locations, not values. These are either the LHSs of
ASG
s or the operands ofADDR
nodes (they can be also be chained viaCOMMA
s). We cannot create non-null assertions from such nodes because they do not end up dereferencing their operands.The problem is that we also cannot detect them reliably. The optimization phases use the
GTF_IND_ASG_LHS
flag, but that is not strong enough as it does not account forADDR
operands (indeed, we "evaluate" these nodes in VN, wasting memory and potentially confusing people who are new to the compiler). Adding a new flag that will capture the general idea of such a location indirection seems very risky and cumbersome to me, as the flag would need to be maintained throughout all the front-end phases (and the maintenance of such flags is known to be a source of bugs).Thus, the solution I propose is a conservative one, but correct: rely on
GTF_NO_CSE
. All locations will be marked with it, and it has already been "plumbed through" the compiler.Contributes to #13762.
Mixed diffs: the change unblocks some CSEs (because their defs now have the exception sets of their uses).