JIT: Make BasicBlock jump target private #93152
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -514,21 +514,28 @@ struct BasicBlock : private LIR::Range | |
|
|
||
| BBjumpKinds bbJumpKind; // jump (if any) at the end of this block | ||
|
|
||
| /* The following union describes the jump target(s) of this block */ | ||
| union { | ||
| unsigned bbJumpOffs; // PC offset (temporary only) | ||
| BasicBlock* bbJumpDest; // basic block | ||
| BBswtDesc* bbJumpSwt; // switch descriptor | ||
| }; | ||
|
|
||
| public: | ||
| BBjumpKinds GetJumpKind() const | ||
| { | ||
| return bbJumpKind; | ||
| } | ||
|
|
||
| void SetJumpKind(BBjumpKinds jumpKind DEBUG_ARG(Compiler* compiler)) | ||
| { | ||
| #ifdef DEBUG | ||
| // BBJ_NONE should only be assigned when optimizing jumps in Compiler::optOptimizeLayout | ||
| // TODO: Change assert to check if compiler is in appropriate optimization phase to use BBJ_NONE | ||
| // (right now, this assertion does the null check to avoid unused variable warnings) | ||
| assert((jumpKind != BBJ_NONE) || (compiler != nullptr)); | ||
| #endif // DEBUG | ||
| bbJumpKind = jumpKind; | ||
| } | ||
|
|
||
| BasicBlock* Prev() const | ||
|
|
@@ -569,12 +576,12 @@ struct BasicBlock : private LIR::Range | |
| return (bbNext == nullptr); | ||
| } | ||
|
|
||
| bool PrevIs(const BasicBlock* block) const | ||
| { | ||
| return (bbPrev == block); | ||
| } | ||
|
|
||
| bool NextIs(const BasicBlock* block) const | ||
| { | ||
| return (bbNext == block); | ||
| } | ||
|
|
@@ -583,12 +590,61 @@ struct BasicBlock : private LIR::Range | |
|
|
||
| bool IsFirstColdBlock(Compiler* compiler) const; | ||
|
|
||
| unsigned GetJumpOffs() const | ||
| { | ||
| return bbJumpOffs; | ||
| } | ||
|
|
||
| void SetJumpOffs(unsigned jumpOffs) | ||
| { | ||
| bbJumpOffs = jumpOffs; | ||
| } | ||
|
|
||
| BasicBlock* GetJumpDest() const | ||
| { | ||
| return bbJumpDest; | ||
|
There was a problem hiding this comment. I wanted to take these new methods as an opportunity to assert we are accessing the jump target correctly. For example, we should only read bbJumpDest if bbJumpKind is valid, and bbJumpDest should not be null for such blocks (and if we can assert bbJumpDest is only accessed when bbJumpKind is valid, then perhaps there's no need to set bbJumpDest to null when setting bbJumpKind to something like BBJ_NONE). However, these asserts turned out to be overly aggressive -- we seem to have some edge cases in a couple of phases that break these assumptions. For one, we do set and compare bbJumpDest to null in a few places, and checking if the jump kind is valid instead of null-checking bbJumpDest would probably hurt TP (as we would have to compare bbJumpKind to multiple possible valid values, versus just one null check). Would it be worth trying to strengthen our bbJumpKind/bbJumpDest abstraction if it comes at the cost of TP? There was a problem hiding this comment. As a next step, this could have an assert for those kinds where bbJumpDest is legal: There was a problem hiding this comment. I wanted to include that too, but this proved to be overly aggressive in a few spots. For example, in Compiler::fgFindBlockILOffset(), we check curr's bbJumpDest as long as it's not a switch statement. I could check if curr's jump kind is valid first to satisfy the assert in GetJumpDest(), but I'm not sure what our stance is on reducing TP to satisfy this constraint. |
||
| } | ||
|
|
||
| void SetJumpDest(BasicBlock* jumpDest) | ||
| { | ||
| bbJumpDest = jumpDest; | ||
|
There was a problem hiding this comment. maybe the same assert here, which would assume the bbJumpKind is set before changing/setting the bbJumpDest. |
||
| } | ||
|
|
||
| void SetJumpKindAndTarget(BBjumpKinds jumpKind, BasicBlock* jumpDest) | ||
| { | ||
| assert(jumpDest != nullptr); | ||
| bbJumpKind = jumpKind; | ||
| bbJumpDest = jumpDest; | ||
| assert(KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_LEAVE)); | ||
| } | ||
|
|
||
| bool HasJumpTo(const BasicBlock* jumpDest) const | ||
| { | ||
| return (bbJumpDest == jumpDest); | ||
|
There was a problem hiding this comment. Same assert here. I wonder if we should treat switch and non-switch more uniformly. i.e., There was a problem hiding this comment. Asserting bbJumpKind is valid proved to be too aggressive in JumpsTo(), as well. Should I try enabling these asserts in a follow-up PR? I think that refactor will be nontrivial, since we null-check bbJumpDest for non-jump blocks in a bunch of places. There was a problem hiding this comment. A follow-up PR to add asserts is fine. There was a problem hiding this comment. It seems odd that If you added a |
||
| } | ||
|
|
||
| bool JumpsToNext() const | ||
| { | ||
| return (bbJumpDest == bbNext); | ||
| } | ||
|
|
||
| BBswtDesc* GetJumpSwt() const | ||
| { | ||
| return bbJumpSwt; | ||
| } | ||
|
|
||
| void SetJumpSwt(BBswtDesc* jumpSwt) | ||
| { | ||
| bbJumpSwt = jumpSwt; | ||
|
There was a problem hiding this comment. Also There was a problem hiding this comment. I thought about this, but we have several instances where we set bbJumpSwt before updating bbJumpKind, and I wouldn't want to enforce an order of the function calls -- though maybe that means we should have a function for converting the block to BBJ_SWITCH that updates bbJumpKind and bbJumpSwt simultaneously? There was a problem hiding this comment.
Maybe an overloaded: for those cases where both are being set? There was a problem hiding this comment. Sure thing, I'll add that in. |
||
| } | ||
|
|
||
| void SetJumpKindAndTarget(BBjumpKinds jumpKind, BBswtDesc* jumpSwt) | ||
| { | ||
| assert(jumpKind == BBJ_SWITCH); | ||
| assert(jumpSwt != nullptr); | ||
| bbJumpKind = jumpKind; | ||
| bbJumpSwt = jumpSwt; | ||
| } | ||
|
|
||
| BasicBlockFlags bbFlags; | ||
|
|
||
|
|
@@ -1617,7 +1673,7 @@ inline BBArrayIterator BBSwitchTargetList::end() const | |
| inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) | ||
| { | ||
| assert(block != nullptr); | ||
| switch (block->GetJumpKind()) | ||
| { | ||
| case BBJ_THROW: | ||
| case BBJ_RETURN: | ||
|
|
@@ -1633,7 +1689,7 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) | |
| case BBJ_ALWAYS: | ||
| case BBJ_EHCATCHRET: | ||
| case BBJ_LEAVE: | ||
| m_succs[0] = block->GetJumpDest(); | ||
| m_begin = &m_succs[0]; | ||
| m_end = &m_succs[1]; | ||
| break; | ||
|
|
@@ -1650,23 +1706,23 @@ inline BasicBlock::BBSuccList::BBSuccList(const BasicBlock* block) | |
|
|
||
| // If both fall-through and branch successors are identical, then only include | ||
| // them once in the iteration (this is the same behavior as NumSucc()/GetSucc()). | ||
| if (block->JumpsToNext()) | ||
| { | ||
| m_end = &m_succs[1]; | ||
| } | ||
| else | ||
| { | ||
| m_succs[1] = block->GetJumpDest(); | ||
| m_end = &m_succs[2]; | ||
| } | ||
| break; | ||
|
|
||
| case BBJ_SWITCH: | ||
| // We don't use the m_succs in-line data for switches; use the existing jump table in the block. | ||
| assert(block->GetJumpSwt() != nullptr); | ||
| assert(block->GetJumpSwt()->bbsDstTab != nullptr); | ||
| m_begin = block->GetJumpSwt()->bbsDstTab; | ||
| m_end = block->GetJumpSwt()->bbsDstTab + block->GetJumpSwt()->bbsCount; | ||
| break; | ||
|
|
||
| default: | ||
|
|
||
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.
You could imagine that
BBJ_CONDcould have, here,struct { BasicBlock* bbJumpTrue; BasicBlock* bbJumpFalse; }where the true case would currently map tobbJumpDestand the false case tobbNext. And have accessors for each. Maybe that's where this will evolve?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 think that's Andy's plan. He mentions in #93020 adding an explicit fall-through successor pointer to BasicBlock that would apply to BBJ_ALWAYS, BBJ_COND, etc. Would it be worth including that work in this PR, or save it for the next one? That change will probably introduce some TP diffs...
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.
Let's hold off... where I'd really like to get to is that these references are either:
The appeal of BasicBlockReference is that someday perhaps I'd like to be able to enumerate all the things that refer a block, including "external" BasicBlock references as well (references from the EH table, from SSA defs, special block references from the compiler object (fgFirstBB, etc), etc).
We could then eliminate all the fussing about with bbRefCounts and either have them maintained automatically or recomputed when needed.
The appeal of FlowEdges is that it simplifies some of the forward likelihood analysis, currently to find an edge to a successor you need to find the successor block and then enumerate the predecessor edges.
But not sure I want to do any of this just yet.
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.
While we are discussing successor edges, I'd like to bring up the fact there is duplication between IR control flow nodes such as
SWITCH/JTRUEand the block kinds. In some systems, information about successors is determined purely from the IR and "terminator" nodes.It is an appealing system from the orthogonality/elegance perspective since there is no duplication, but it is unlikely to be suitable for us due to the throughput cost (of at least two additional indirections in HIR).