[PATCH] Initial instrumentation based PGO implementation

[Justin Bogner]

Eric Christopher <echristo at gmail.com> writes:
> Just a few things here... 
>
> +  CGF.EmitBranchOnBoolExpr(E->getConfig(), ContBlock, ConfigOKBlock, 0);
>
> Entirely up to you, but I find that commenting options like the 0 there with
> "Initial value" or something is useful when you're not passing an obvious
> argument along that has a name.

Sure.

> +      if (CondExprBool)
> +        Cnt.beginRegion(Builder);
>
> I'd probably want a comment above the conditional here.

Sure.

> +  PGO.setCurrentRegionCount(0);
>
> Perhaps resetCurrentRegionCount to do the explicit 0? It's more obvious what's
> going on then.

I had that in a previous iteration, and explicitly setting the count
ends up clearer. Reset is ambiguous, whereas "the region count is zero
here" clearly indicates this region shouldn't occur.

> +  Cnt.adjustFallThroughCount();
>
> This is weird - I'll mention it later :)
>
> +    uint64_t Total = CaseCnt.getCount() - CaseCnt.getParentCount();
> +    unsigned NCases = Range.getZExtValue() + 1;
> +    uint64_t Weight = Total / NCases, Rem = Total % NCases;
> +    for (unsigned I = 0; I != NCases; ++I) {
> +      if (SwitchWeights)
> +        SwitchWeights->push_back(Weight + (Rem ? 1 : 0));
> +      if (Rem)
> +        Rem--;
>        SwitchInsn->addCase(Builder.getInt(LHS), CaseDest);
>
> Should comment what you're doing here.

Sure.

> +    (*SwitchWeights)[0] += ThisCount;
>
> This seems awkward.

That's why there's a big comment explaining it.

> +  llvm::BasicBlock *SkipCountBB = 0;
> +  if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
> +    SkipCountBB = createBasicBlock("skipcount");
> +    EmitBranch(SkipCountBB);
> +  }
>
> Probably want a comment of why you're emitting this block.

Sure.

> +    const SwitchCase *Case = S.getSwitchCaseList();
> +    uint64_t DefaultCount = 0;
> +    unsigned NumCases = 0;
> +    for (; Case; Case = Case->getNextSwitchCase()) {
>
> No reason to have Case outside of the loop as far as I can tell?

Sure.

> -                            llvm::BasicBlock *FalseBlock);
> +                            llvm::BasicBlock *FalseBlock, uint64_t
> TrueCount);
>
> Comment what TrueCount is above.

Sure.

> +  void emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter) {
> +    if (!CGM.getCodeGenOpts().ProfileInstrGenerate)
> +      return;
> +    llvm::Value *Addr =
> +      Builder.CreateConstInBoundsGEP2_64(RegionCounters, 0, Counter);
> +    llvm::Value *Count = Builder.CreateLoad(Addr, "pgocount");
> +    Count = Builder.CreateAdd(Count, Builder.getInt64(1));
> +    Builder.CreateStore(Count, Addr);
> +  }
>
> Probably a bit big to have in the header.

Sure, moved.

> +  /// Get the value of the counter with adjustments applied. Adjustments are
> +  /// applied whenever control flow enters or leaves the region abnormally.
> +  uint64_t getAdjustedCount() const {
> +    assert(Adjust > 0 || (uint64_t)(-Adjust) <= Count && "Negative count");
> +    return Count + Adjust;
> +  }
>
> Few things with this:
>
> a) Adjustments seem awkward, possibly need to describe this better.
> b) You use accessor functions in all of the other functions.

I've improved the comment. Adjust doesn't have an accessor, since it
isn't public, and mixing getCount() with Adjust is awkward.

> +  /// Get the value of the counter that was active before this one.
> +  uint64_t getParentCount() const { return ParentCount; }
>
> Parent count seems a bit awkward for naming. I've seen the uses through the
> source, but some documentation of what's going on here and why you'd want to
> get it would be good.

I've improved the comment.

> +  /// Get the associated break counter. Undefined if this counter is not
> +  /// counting a loop.
> +  RegionCounter getBreakCounter() const {
> +    return RegionCounter(*PGO, Counter + 1);
> +  }
> +  /// Get the associated continue counter. Undefined if this counter is not
> +  /// counting a loop.
> +  RegionCounter getContinueCounter() const {
> +    return RegionCounter(*PGO, Counter + 2);
> +  }
>
> "Undefined" boo! Any way to make it do something sane or assert?

It's possible to make this assert, but it's a bit messy. It's pretty
likely that these will go away completely in follow on work, so I think
it's best to address this outside of this review.

> +  void setFallThroughCount() {
>
> Possibly "finishAdjustments" or something like that? A set that doesn't take
> an argument is a bit odd. The "adjustFallThroughCount()" code is also weird. I
> like abstracting the idea out, but I'm not sure how it should be done here.

I went with applyAdjustmentsToRegion.

> The testcases look much better. I'm a bit astounded that you can map them to
> the source quite like that, but hey, awesome that you can.
>
> Perhaps having a comment operator in the input file format would be nice so
> that you could comment the input files, but that's definitely an "in the
> future" sort of thing.
>
> -eric

I've attached a patch with just the incremental changes here, and as you
can see, there aren't any functional changes. Since we're basically just
improving comments at this point, and since this not being in has been
blocking other people trying to use and build on top of these changes,
I'm going to go ahead and commit now. I'll continue to incorporate
further feedback in follow on commits.

Thanks for the review!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: incremental-changes.patch
Type: text/x-patch
Size: 11048 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140106/3b420d50/attachment.bin>