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

[Vedant Kumar via Phabricator via llvm-commits]

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.



================
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