[PATCH] Initial instrumentation based PGO implementation

[Eric Christopher]

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.

+      if (CondExprBool)
+        Cnt.beginRegion(Builder);

I'd probably want a comment above the conditional here.

+  PGO.setCurrentRegionCount(0);

Perhaps resetCurrentRegionCount to do the explicit 0? It's more obvious
what's going on then.

+  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.

+    (*SwitchWeights)[0] += ThisCount;

This seems awkward.

+  llvm::BasicBlock *SkipCountBB = 0;
+  if (CGM.getCodeGenOpts().ProfileInstrGenerate) {
+    SkipCountBB = createBasicBlock("skipcount");
+    EmitBranch(SkipCountBB);
+  }

Probably want a comment of why you're emitting this block.

+    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?

-                            llvm::BasicBlock *FalseBlock);
+                            llvm::BasicBlock *FalseBlock, uint64_t
TrueCount);

Comment what TrueCount is above.


+  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.

+  /// 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.

+  /// 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.

+  /// 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?

+  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.

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

On Sun Jan 05 2014 at 6:34:13 PM, Eric Christopher <echristo at gmail.com>
wrote:

> Will get to this tomorrow.
> On Jan 5, 2014 6:33 PM, "Justin Bogner" <mail at justinbogner.com> wrote:
>
> ping...
>
> Eric Christopher <echristo at gmail.com> writes:
> > Must have slipped off of my radar - especially with the holidays.
> > Thanks for the ping.
> >
> > -eric
> >
> > On Sun, Dec 29, 2013 at 8:49 PM, Bob Wilson <bob.wilson at apple.com>
> wrote:
> >> Eric,
> >>
> >> Thanks for your careful review of this.  Your comments about the
> >> testing strategy have been especially helpful. This is an initial
> >> patch that is going to be revised extensively before the feature is
> >> finished. There are a lot of incremental changes that need to
> >> happen, and at least for myself, I have been unable to contribute
> >> any of those changes while we wait to get this initial patch
> >> committed. It has now been almost a month since Justin first
> >> submitted this patch (Dec. 1). I have carefully reviewed it twice,
> >> and John (the code owner) has also reviewed it. Can you please give
> >> an OK to let Justin commit this patch?
> >>
> >> On Dec 19, 2013, at 12:37 AM, Justin Bogner <mail at justinbogner.com>
> wrote:
> >>
> >>> Eric Christopher <echristo at gmail.com> writes:
> >>>> I don't think we should have any executable tests in the front end at
> all. I
> >>>> think the easiest way here would be to check in an input file
> alongside the
> >>>> test file similar to how the Object tests work (an Inputs directory).
> >>>>
> >>>> Thoughts?
> >>>
> >>> I'm a bit leery of input files, especially since the file format for
> the
> >>> PGO stuff is explicitly in flux here. That said, writing tests the way
> >>> you suggest has a number of advantages and tests that only sometimes
> run
> >>> are clearly inferior.
> >>>
> >>> So I went ahead and ripped out the profile-generate part, added an
> input
> >>> file for the profile-use part, and even added a test that we ignore
> >>> bogus data, which was impossible with the previous approach.
> >>>
> >>> Doing so pointed out the problem with this change. The tests I had were
> >>> testing two things: generating profile data, and using it. Using an
> >>> input file was only the latter. That's lame, so I've added a second run
> >>> line that spits out IR and checks that we're incrementing the
> >>> appropriate counters for the various constructs.
> >>>
> >>> In short, this makes the tests *way* better. They're twice as
> >>> complicated, but they're testing twice as much stuff. Check it out.
> >>>
> >>>
> <0002-CodeGen-Initial-instrumentation-based-PGO-implementa.patch>_______________________________________________
> >>> cfe-commits mailing list
> >>> cfe-commits at cs.uiuc.edu
> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140106/20cfb1d6/attachment.html>