-
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
Spanning tree instrumentation #47959
Changes from 3 commits
1bf2405
9165fb5
191a339
658a75b
cb025c1
26ce9e5
7ce99ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -882,9 +882,14 @@ struct BasicBlock : private LIR::Range | |
void ensurePredListOrder(Compiler* compiler); | ||
void reorderPredList(Compiler* compiler); | ||
|
||
BlockSet bbReach; // Set of all blocks that can reach this one | ||
BasicBlock* bbIDom; // Represent the closest dominator to this block (called the Immediate | ||
// Dominator) used to compute the dominance tree. | ||
BlockSet bbReach; // Set of all blocks that can reach this one | ||
|
||
union { | ||
BasicBlock* bbIDom; // Represent the closest dominator to this block (called the Immediate | ||
// Dominator) used to compute the dominance tree. | ||
void* bbProbeList; // Used early on by fgInstrument | ||
void* bbInfo; // Used early on by fgIncorporate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Could you use a more specific name than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed. |
||
}; | ||
|
||
unsigned bbPostOrderNum; // the block's post order number in the graph. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,7 +170,10 @@ void Compiler::fgInit() | |
fgPgoSchemaCount = 0; | ||
fgNumProfileRuns = 0; | ||
fgPgoBlockCounts = 0; | ||
fgPgoEdgeCounts = 0; | ||
fgPgoClassProfiles = 0; | ||
fgCountInstrumentor = nullptr; | ||
fgClassInstrumentor = nullptr; | ||
fgPredListSortVector = nullptr; | ||
} | ||
|
||
|
@@ -450,7 +453,10 @@ void Compiler::fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* ne | |
{ | ||
// Remove the old edge [oldTarget from blockSwitch] | ||
// | ||
fgRemoveAllRefPreds(oldTarget, blockSwitch); | ||
if (fgComputePredsDone) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the header comment that says "We also must update the predecessor lists for 'oldTarget' and 'newPred'."? It seems like our basic manipulation routines need to document if they can run without preds, maybe with an appropriate assert if they can't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missed this one in my first review feedback commit. |
||
{ | ||
fgRemoveAllRefPreds(oldTarget, blockSwitch); | ||
} | ||
|
||
// | ||
// Change the jumpTab entry to branch to the new location | ||
|
@@ -460,7 +466,12 @@ void Compiler::fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* ne | |
// | ||
// Create the new edge [newTarget from blockSwitch] | ||
// | ||
flowList* newEdge = fgAddRefPred(newTarget, blockSwitch); | ||
flowList* newEdge = nullptr; | ||
|
||
if (fgComputePredsDone) | ||
{ | ||
newEdge = fgAddRefPred(newTarget, blockSwitch); | ||
} | ||
|
||
// Now set the correct value of newEdge->flDupCount | ||
// and replace any other jumps in jumpTab[] that go to oldTarget. | ||
|
@@ -479,7 +490,10 @@ void Compiler::fgReplaceSwitchJumpTarget(BasicBlock* blockSwitch, BasicBlock* ne | |
// | ||
// Increment the flDupCount | ||
// | ||
newEdge->flDupCount++; | ||
if (fgComputePredsDone) | ||
{ | ||
newEdge->flDupCount++; | ||
} | ||
} | ||
i++; // Check the next entry in jumpTab[] | ||
} | ||
|
@@ -1780,11 +1794,6 @@ unsigned Compiler::fgMakeBasicBlocks(const BYTE* codeAddr, IL_OFFSET codeSize, F | |
} | ||
} | ||
|
||
if (compIsForInlining()) | ||
{ | ||
fgComputeProfileScale(); | ||
} | ||
|
||
do | ||
{ | ||
unsigned jmpAddr = DUMMY_INIT(BAD_IL_OFFSET); | ||
|
@@ -3591,22 +3600,27 @@ BasicBlock* Compiler::fgSplitBlockAtBeginning(BasicBlock* curr) | |
// 'succ' might be the fall-through path or the branch path from 'curr'. | ||
// | ||
// Arguments: | ||
// curr - A block which branches conditionally to 'succ' | ||
// curr - A block which branches to 'succ' | ||
// succ - The target block | ||
// | ||
// Return Value: | ||
// Returns a new block, that is a successor of 'curr' and which branches unconditionally to 'succ' | ||
// | ||
// Assumptions: | ||
// 'curr' must have a bbJumpKind of BBJ_COND or BBJ_SWITCH | ||
// 'curr' must have a bbJumpKind of BBJ_COND, BBJ_ALWAYS, or BBJ_SWITCH | ||
// | ||
// Notes: | ||
// The returned block is empty. | ||
// Can be invoked before pred lists are built. | ||
|
||
BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) | ||
{ | ||
assert(curr->bbJumpKind == BBJ_COND || curr->bbJumpKind == BBJ_SWITCH); | ||
assert(fgGetPredForBlock(succ, curr) != nullptr); | ||
assert(curr->bbJumpKind == BBJ_COND || curr->bbJumpKind == BBJ_SWITCH || curr->bbJumpKind == BBJ_ALWAYS); | ||
|
||
if (fgComputePredsDone) | ||
{ | ||
assert(fgGetPredForBlock(succ, curr) != nullptr); | ||
} | ||
|
||
BasicBlock* newBlock; | ||
if (succ == curr->bbNext) | ||
|
@@ -3639,20 +3653,30 @@ BasicBlock* Compiler::fgSplitEdge(BasicBlock* curr, BasicBlock* succ) | |
} | ||
fgAddRefPred(newBlock, curr); | ||
} | ||
else | ||
else if (curr->bbJumpKind == BBJ_SWITCH) | ||
{ | ||
assert(curr->bbJumpKind == BBJ_SWITCH); | ||
|
||
// newBlock replaces 'succ' in the switch. | ||
fgReplaceSwitchJumpTarget(curr, newBlock, succ); | ||
|
||
// And 'succ' has 'newBlock' as a new predecessor. | ||
fgAddRefPred(succ, newBlock); | ||
} | ||
else | ||
{ | ||
assert(curr->bbJumpKind == BBJ_ALWAYS); | ||
fgReplacePred(succ, curr, newBlock); | ||
curr->bbJumpDest = newBlock; | ||
newBlock->bbFlags |= BBF_JMP_TARGET; | ||
fgAddRefPred(newBlock, curr); | ||
} | ||
|
||
// This isn't accurate, but it is complex to compute a reasonable number so just assume that we take the | ||
// branch 50% of the time. | ||
newBlock->inheritWeightPercentage(curr, 50); | ||
// | ||
if (curr->bbJumpKind != BBJ_ALWAYS) | ||
{ | ||
newBlock->inheritWeightPercentage(curr, 50); | ||
} | ||
|
||
// The bbLiveIn and bbLiveOut are both equal to the bbLiveIn of 'succ' | ||
if (fgLocalVarLivenessDone) | ||
|
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.
Do you have something that nulls these out before the phases that uses them? Or initializes them before they are read? I see somewhere that you assert bbInfo is != nullptr, for instance.
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.
For instrumentation this happens in the
VisitBlock
method during the tree walk. For reconstruction the value is set duringPrepare
before it is read.