[PATCH] D62700: [PGO] Handle cases of non-instrument BBs

David Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 10 09:33:24 PDT 2019


davidxl added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1090
+  }
+  ProfileCountSize = CountFromProfile.size();
+  CountPosition = I;
----------------
xur wrote:
> davidxl wrote:
> > xur wrote:
> > > davidxl wrote:
> > > > 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?
> > > 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. 
> > > With that assumption, all the instrumented edges will have a valid count.  After that we can populate the count to all the remaining 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.
> > > I fix this by setting counts in two steps: first step to read all the counts in profile files to set count for the instrumented BB.
> > > In the second step, I check all the instrumented edges: I should get the counts from source or sink. Otherwise, it should be case of not able to instrument. I set the count to 0.
> > > 
> > > 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.
> > > 
> > > 
> > 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?
> InEdges is initialized after the first step.  So that piece code is effectively dead code.
> 
> 
Can the InEdges/OutEdges be set  in getInstrumentedBBs? It seems a natural place to set the CFG after critical edge split.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62700/new/

https://reviews.llvm.org/D62700





More information about the llvm-commits mailing list