-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[TODO-List-Cleanup] Delete NumChildren
, GetChild
and gtGetChildPointer
#61875
[TODO-List-Cleanup] Delete NumChildren
, GetChild
and gtGetChildPointer
#61875
Conversation
TryGetUse does the same thing and is used more often. Also some editorial renames to standardize on the "user/use" terminology.
In most situations these methods are the wrong thing to call - they are not very efficient by design. Not surprisingly, thus, they were mostly only used in debug code. The few product dependencies were removed and replaced with equivalent alternatives, deduplicating the code. In the process, BasicBlock::firstNode() was removed as it was not used anywhere.
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsAll these functions represent what is essentially duplicated code.
|
@dotnet/jit-contrib |
@AndyAyersMS PTAL. |
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.
Looks good overall, just one nit on parameter naming.
If you change that it, might ripple through to other similar APIs.
src/coreclr/jit/gentree.cpp
Outdated
@@ -4986,199 +4986,20 @@ unsigned GenTree::GetScaledIndex() | |||
} | |||
|
|||
//------------------------------------------------------------------------ | |||
// gtGetChildPointer: If 'parent' is the parent of this node, return the pointer | |||
// to the child node so that it can be modified; otherwise, return nullptr. | |||
// TryGetUse: Get the def -> def edge for a child of this tree. |
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.
Nit: def
seems like an odd name choice here, and def -> def
likewise.
Maybe child
would be better?
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.
Renamed to operand
as that's what other related APIs use (ReplaceOperand
, VisitOperands
, Operands
).
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.
Thanks, that reads much better.
Did you look at JIT throughput with this change? Especially for Tier0 code and or Tier0 / w PGO? Techempower runs show a notable drop in full PGO time to first response on around 11/30-12/1 and from a quick scan, this seems to be the only change that might be relevant. But a drop of this magnitude also seems a bit hard to believe... |
@AndyAyersMS I've just checked - it's not this commit (I've checked the commit prior this one)
which is 541 (already improved result) |
All these functions represent what is essentially duplicated code.
No diffs.