-
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
Don't bail out on side-effects in assertionprop #108418
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@dotnet/jit-contrib @AndyAyersMS PTAL, small change, Diffs TLDR: We have In order to allow arg0 to have side-effects. we simply create a temp inside LateArg and use that temp. Thus, PS: I recommend reviewing with "Hide whitespaces" option. |
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.
Changes LGTM.
Since we're in the middle of VN/SSA phases, I wonder if it would make sense to build a version of fgMakeMultiUse
that puts the new temp into SSA and gives it the right VNs.
if ((castObjArg->gtFlags & GTF_ALL_EFFECT) != 0) | ||
{ | ||
// It won't be trivial to properly extract side-effects from the call node. | ||
// Ideally, we only need side effects from the castClsArg argument as the call itself | ||
// won't throw any exceptions. But we should not forget about the EarlyNode (setup args) | ||
return nullptr; | ||
} |
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.
Why would gtExtractSideEffList(call, ..., /* ignoreRoot */ true)
not be sufficient here? I don't think extracting the side effects of the operands of a call is all that hard.
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.
@jakobbotsch I am not sure I understand how it can work, say I have
call(arg0, arg1, arg2)
I want to fold it to just arg1
(or to e.g. arg1*arg1
in case of Math.Pow(x, 2)
) and say arg1 has a side-effect in it.
if we just return arg1 node and wrap with gtExtractSideEffList
we'll just duplicate the side effect
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.
Ah okay, I see -- that makes sense.
No description provided.