-
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
Zero-diff part of "Use SSA-based ComputeRange" #112853
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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.
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
@jakobbotsch @AndyAyersMS @dotnet/jit-contrib PTAL, zero-diff change. Just preparations for using RangeCheck in assertprop (in #112824):
CI failures are unrelated. |
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.
I guess we're trying to economize by keeping the collection backbones around once we've allocated them.
Could we make it clearer which parts of the range check state are durable and which parts are not? Eg split out the more transient parts into a caller-supplied struct or something?
Or make it harder to accidentally call into the wrong part of range check?
Range GetRange(BasicBlock* block, GenTree* expr); | ||
|
||
// Internal worker for GetRange. | ||
Range GetRangeWorker(BasicBlock* block, GenTree* expr, bool monIncreasing DEBUGARG(int indent)); |
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.
Should this now be private? Ditto for some of the other helpers.
@@ -365,7 +381,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree* | |||
} | |||
|
|||
JITDUMP("Range value %s\n", range.ToString(m_pCompiler)); | |||
m_pSearchPath->RemoveAll(); | |||
GetSearchPath()->RemoveAll(); |
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.
This part reads a bit oddly -- if we don't have a search path we create one and then immediately remove everything in it? Maybe EnsureEmpty
? which first checks if the thing is allocated?
Extracted from #112824 for simpler code review