-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix side-effect extraction in optVNBasedFoldConstExpr #117599
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
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
PTAL @dotnet/jit-contrib @jakobbotsch correctness change, I've hit it with Diffs, some improvements from the GT_ARR_LENGTH -> GT_NULLCHECK folding |
Are the extra null checks in win-x86 diffs expected? |
I've realized that yeah, there are too many nullchecks in the diffs. The problem that in VN phase we have a bunch of optimizations that look through static readonly fields, RVA, etc - in some of those cases we actually know that the indirect is never faulty, but since we don't mutate the IR in the VN phase, we don't have a way to tell that to the assertprop phase. I'll put on hold this change Even just ROS<char> x = "hello";
var val = x[1]; now produces a nullcheck. |
VN-based constant propagation used to incorrectly handle side-effects - it used to pass "ignoreTrue = true" to
gtWrapWithSideEffects
. This could led to replacingIND(node)
with a constant while ignoring the GTF_EXCEPT on the IND node itself (it's precisely what I've hit in #117583). Looks like most regressions from this change were caused by GT_ARR_LENGTH node so I made a small optimization ingtExtractSideEffList
to fold unused GT_ARR_LENGTH into GT_NULLCHECK to mitigate some regressions.