As shown in PR41279, some basic blocks (such as catchswitch) cannot be instrumented.
This patch filters out these BBs in PGO instrumentation.
Details
Diff Detail
Event Timeline
Another change of this patch is to move down statistic collection code for critical edge. I think this is a better place.
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
801 | Add a comment here explaining the predicate. |
My previous patch was not complete -- we need to set the profile count to the fail-to-instrument edge. Otherwise, we cannot propagate the counts in the CFG.
The new patch has this part of change.
Updated the test to test the profile-use.
Also added comments suggested by David.
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | The change seems pretty substantial. Is it enough just do one pass and set all edges not already with a valid count to zero count? |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | After MST, we mark the edges that will be instrumented. The instrumentation needs to be done in BB. So we have getInstrBB() to decide which BB to instrument. in getInstrBB, we split BB for critical edges, so edges can be added and remove. BB could be added too. Previous implementation assumes that all edges that marked instrument will be instrumented. So we have one pass that sets the instrumented BB and the instrumented edges. Now, some of the BBs that supposed to instrumented has no count. Some of the instrumented edges have no count, so the population fails. Doing this in one step is more tricky. Note that previously set the population data structure (OutEdges, InEdges) after the getInstrBB(). This way we don't need to update them for split BBs. I think doing this in two steps simplifies the code and is clearly conceptually. We probably should not mark all edges as valid in reading count step. This could mask some errors in count population. The valid bit should be set by count population, I think. |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | I understand the need to do two pass annotation, but in the first pass when BB is annotated with count, setting inedge and outedge count like if (Info.InEdges.size() == 1) { .... } is still correct. What I meant is that why can't the second pass be as simple as just mark remaining edges (not annotated in the first pass) with zero count? |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | InEdges is initialized after the first step. So that piece code is effectively dead code. |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | Can the InEdges/OutEdges be set in getInstrumentedBBs? It seems a natural place to set the CFG after critical edge split. |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | Do you mean setting the InEdges/OutEdges before getInstrumentedBBs() and updating them when splitting? I chose not to that for two reasons: |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | No need to update. Just split the following from popluteCounters and put it at the end of getInstrumentedBBs: for (auto &E : FuncInfo.MST.AllEdges) { if (E->Removed) continue; const BasicBlock *SrcBB = E->SrcBB; const BasicBlock *DestBB = E->DestBB; UseBBInfo &SrcInfo = getBBInfo(SrcBB); UseBBInfo &DestInfo = getBBInfo(DestBB); SrcInfo.OutEdges.push_back(E.get()); DestInfo.InEdges.push_back(E.get()); } |
llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp | ||
---|---|---|
1120 | getInstrumentedBBs() is a template funciton. The one instantiated for instrumentation does not have InEdges/OutEdges (it uses BBInfo). We would need a specialization for UseBBInfo type. |
This can be handled this way:
BBInfo &SrcInfo = getBBInfo(SrcBB); // BBInfo is the template type BBInfo &DestInfo = getBBInfo(DestBB); SrcInfo.addOutEdge(E.get()); DestInfo.addInEdge(E.get());
For base class BBInfo, addOutEdge, addInEdge will be empty, while it is defined for UseBBInfo class.
By so doing, the code is more readable.
Add a comment here explaining the predicate.