JIT: Replace BlockSet with BitVec, and remove BB epoch
#110034
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib, @jakobbotsch PTAL. No diffs. Thanks! |
Yes. I can include that in this PR, if you'd like. |
amanasifkhalid
changed the title
BlockSet usages with BitVecBlockSet with BitVec, and remove BB epoch
src/coreclr/jit/block.cpp
Outdated
| unsigned bbNumMax = compiler->fgBBNumMax; | ||
| BitVecTraits bitVecTraits(bbNumMax + 1, compiler); | ||
| BitVec uniqueSuccBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); | ||
| unsigned* const targetNums = new unsigned[NumSucc()]; |
There was a problem hiding this comment.
| unsigned* const targetNums = new unsigned[NumSucc()]; | |
| unsigned* const targetNums = new (compiler, CMK_DebugOnly) unsigned[NumSucc()]; |
But is this change even necessary?
There was a problem hiding this comment.
I guess not -- if we get to the point where we don't need bbNum at all, we can rewrite this logic altogether to print in bbID order (or some other order).
src/coreclr/jit/fgdiagnostic.cpp
Outdated
| // Create a set with all the successors. | ||
| BitVecTraits bitVecTraits(compBasicBlockID, this); | ||
| BitVec succBlocks(BitVecOps::MakeEmpty(&bitVecTraits)); | ||
| for (BasicBlock* const bTarget : block->SwitchTargets()) | ||
| { | ||
| BitVecOps::AddElemD(&bitVecTraits, succBlocks, bTarget->bbNum); | ||
| BitVecOps::AddElemD(&bitVecTraits, succBlocks, bTarget->bbID); | ||
| } | ||
| // Now we should have a set of unique successors that matches what's in the switchMap. | ||
| // First, check the number of entries, then make sure all the blocks in uniqueSuccSet | ||
| // are in the BlockSet. | ||
| // are in the BitVec. | ||
| unsigned count = BitVecOps::Count(&bitVecTraits, succBlocks); | ||
| assert(uniqueSuccSet.numDistinctSuccs == count); | ||
| for (unsigned i = 0; i < uniqueSuccSet.numDistinctSuccs; i++) | ||
| { | ||
| assert(BitVecOps::IsMember(&bitVecTraits, succBlocks, | ||
| uniqueSuccSet.nonDuplicates[i]->getDestinationBlock()->bbNum)); | ||
| uniqueSuccSet.nonDuplicates[i]->getDestinationBlock()->bbID)); | ||
| } |
There was a problem hiding this comment.
It's not necessary for removing BlockSet, but these are the last few usages of bbNum-based BitVecs, so I thought I'd get rid of them while I'm here.
There was a problem hiding this comment.
I don't see anything wrong with bitsets based on either bbNum or bbID. I suppose it depends on whether the end goal is to remove bbID in favor of bbNum or bbNum in favor of bbID. I am slightly biased towards keeping the name as bbNum considering that we are going to be using them as indices, and that it has been much more pervasive than bbID in the JIT, but I guess it's unimportant.
I would suggest we make these changes in the PR that eventually unifies the two fields and removes one of them, though.
src/coreclr/jit/fgflow.cpp
Outdated
| { | ||
| nonDups[nonDupInd] = succEdge; | ||
| nonDupInd++; | ||
| BitVecOps::RemoveElemD(&blockVecTraits, uniqueSuccBlocks, targ->bbNum); | ||
| BitVecOps::RemoveElemD(&blockVecTraits, uniqueSuccBlocks, targ->bbID); | ||
| } |
There was a problem hiding this comment.
Ditto here, it seems unnecessary to change this.
There was a problem hiding this comment.
This switch to bbID-based BitVecs isn't Debug-only like the other ones, though the sparser set doesn't seem to have impacted TP all that much.
There was a problem hiding this comment.
Yeah, I would be surprised if the sparser bit sets are going to have any noticeable impact on performance.. A few bits wasted due to optimizing out entire basic blocks is going to be a drop in the bucket compared to everything else that's happening when handling the IR in those basic blocks.
My feedback here probably more comes down to bikeshedding over the naming of the final field we are going to end up with...
|
/ba-g linux-x64 build timed out |
Part of #107749. Prerequisite for #110026.
fgIncorporateProfileData. This runs early enough in the JIT frontend such that I would expect bbIDs and bbNums to be 1:1, so I don't expect any TP impact from this change.Switch descriptor creation still uses
bbNumsas a key into aBitVecas a workaround for BB epoch invariants -- I'll try switching this over tobbIDin a follow-up to evaluate the TP cost of a sparser bitset.