[PATCH] D34085: [PGO] Register promote profile counter updates

[Xinliang David Li via llvm-commits]

On Tue, Jun 13, 2017 at 4:56 PM, Vedant Kumar via Phabricator <
reviews at reviews.llvm.org> wrote:

> vsk added a comment.
>
> In https://reviews.llvm.org/D34085#779375, @davidxl wrote:
>
> > Regarding turning this on for FE instrumentation, it can be done, but
> probably as a follow up (which also needs loop canonlicalization and
> rotation). More coverage test performance data (possibly at O0) also need
> to be collected.
>
>
> I'd be willing to help out with this, unless you already have plans to get
> to it.
>
>
that will be great!

David

>
>
> ================
> Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:170
> +      Instruction *InsertPos = InsertPts[i];
> +      Value *LiveInValue = SSA.GetValueInMiddleOfBlock(ExitBlock);
> +      Value *Addr = cast<StoreInst>(Store)->getPointerOperand();
> ----------------
> This may be a basic question, but.. why is it that
> SSA.GetValueInMiddleOfBlock(ExitBlock) gives the right increment, coming
> from the promoted counter for {L, S}? It may help to have a short comment
> about this for readers who are unfamiliar with SSAUpdater (like me).
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:101
> +cl::opt<bool> AtomicCounterUpdatePromoted(
> +    "atomic-counter-update-promoted", cl::ZeroOrMore,
> +    cl::desc("Do counter update using atomic fetch add "
> ----------------
> davidxl wrote:
> > vsk wrote:
> > > Would cl::Optional be more appropriate?
> > Sometimes the build system can append multiple instances of a common
> flag to the command line. Without ZeroOrMore, driver reports error.
> Ok
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:111
> +// setting this option can override the default behavior.
> +cl::opt<bool> DoCounterPromotion("do-counter-promotion", cl::ZeroOrMore,
> +                                 cl::desc("Do counter register
> promotion"),
> ----------------
> davidxl wrote:
> > vsk wrote:
> > > The "do-counter-promotion" flag is ternary, and should be simplified.
> I'd prefer a cl::opt which specifies whether counter promotion is
> "allowed", and an InstrProfOptions field which specifies whether it is
> done. In promoteCounterLoadStores, you could write:
> > > ```
> > > if (!AllowCounterPromotion || !Opts.DoCounterPromotion)
> > >   return;
> > > ```
> > >
> > We need the ability to use this option to override (either force it to
> be on or off) the default pipeline option. Your proposal does not seem to
> accomplish that.
> I think the 'isCounterPromotionEnabled' method makes things clear enough
>
>
> ================
> Comment at: test/Transforms/PGOProfile/counter_promo.ll:40
> +  ret void
> +; PROMO:  %pgocount.promoted = load {{.*}} @__profc_foo{{.*}} 0)
> +; PROMO-NEXT: add
> ----------------
> davidxl wrote:
> > vsk wrote:
> > > You may need to add 'requires: asserts' to tests which refer to bb or
> variable names.
> > The name of the counter vars seem to be kept with the non-asserts
> compiler. Any idea why?
> Ah, I think ssa var names are kept, but basic block label names are not.
>
>
> ================
> Comment at: test/Transforms/PGOProfile/counter_promo.ll:52
> +; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)
> +
> +}
> ----------------
> davidxl wrote:
> > davidxl wrote:
> > > vsk wrote:
> > > > vsk wrote:
> > > > > vsk wrote:
> > > > > > In this test, the CFG looks like:
> > > > > > ```
> > > > > >     2
> > > > > >    /  \
> > > > > >  4      5
> > > > > >  |     / \
> > > > > >  |    7    8
> > > > > >   \  /____/
> > > > > >     9 --- loop ---> 2
> > > > > >     |
> > > > > >    exit
> > > > > > ```
> > > > > > Is this the simplest loop where ld/st promotion applies?
> > > > > Could you add checks for the operands to the adds? It would be
> useful to have checks for the PHIs incremented in the loop body, to ensure
> they make their way to the correct 'add'.
> > > > Could you add a check-not for further uses of __profc*?
> > > The purpose is to test multiple counters hoisted
> > Done.
> I think this is still missing
>
>
> https://reviews.llvm.org/D34085
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170613/78b81ca9/attachment.html>