<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 13, 2017 at 4:56 PM, Vedant Kumar via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">vsk added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D34085#779375" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34085#779375</a>, @davidxl wrote:<br>
<br>
> 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.<br>
<br>
<br>
</span>I'd be willing to help out with this, unless you already have plans to get to it.<br>
<br></blockquote><div><br></div><div>that will be great!</div><div><br></div><div>David </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/Transforms/<wbr>Instrumentation/<wbr>InstrProfiling.cpp:170<br>
+ Instruction *InsertPos = InsertPts[i];<br>
+ Value *LiveInValue = SSA.GetValueInMiddleOfBlock(<wbr>ExitBlock);<br>
+ Value *Addr = cast<StoreInst>(Store)-><wbr>getPointerOperand();<br>
----------------<br>
This may be a basic question, but.. why is it that SSA.GetValueInMiddleOfBlock(<wbr>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).<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/<wbr>Instrumentation/<wbr>InstrProfiling.cpp:101<br>
+cl::opt<bool> AtomicCounterUpdatePromoted(<br>
+ "atomic-counter-update-<wbr>promoted", cl::ZeroOrMore,<br>
+ cl::desc("Do counter update using atomic fetch add "<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> vsk wrote:<br>
> > Would cl::Optional be more appropriate?<br>
> Sometimes the build system can append multiple instances of a common flag to the command line. Without ZeroOrMore, driver reports error.<br>
</span>Ok<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/<wbr>Instrumentation/<wbr>InstrProfiling.cpp:111<br>
+// setting this option can override the default behavior.<br>
+cl::opt<bool> DoCounterPromotion("do-<wbr>counter-promotion", cl::ZeroOrMore,<br>
+ cl::desc("Do counter register promotion"),<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> vsk wrote:<br>
> > 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:<br>
> > ```<br>
> > if (!AllowCounterPromotion || !Opts.DoCounterPromotion)<br>
> > return;<br>
> > ```<br>
> ><br>
> 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.<br>
</span>I think the 'isCounterPromotionEnabled' method makes things clear enough<br>
<span class=""><br>
<br>
================<br>
Comment at: test/Transforms/PGOProfile/<wbr>counter_promo.ll:40<br>
+ ret void<br>
+; PROMO: %pgocount.promoted = load {{.*}} @__profc_foo{{.*}} 0)<br>
+; PROMO-NEXT: add<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> vsk wrote:<br>
> > You may need to add 'requires: asserts' to tests which refer to bb or variable names.<br>
> The name of the counter vars seem to be kept with the non-asserts compiler. Any idea why?<br>
</span>Ah, I think ssa var names are kept, but basic block label names are not.<br>
<div><div class="h5"><br>
<br>
================<br>
Comment at: test/Transforms/PGOProfile/<wbr>counter_promo.ll:52<br>
+; PROMO-NEXT: store {{.*}}@__profc_foo{{.*}}3)<br>
+<br>
+}<br>
----------------<br>
davidxl wrote:<br>
> davidxl wrote:<br>
> > vsk wrote:<br>
> > > vsk wrote:<br>
> > > > vsk wrote:<br>
> > > > > In this test, the CFG looks like:<br>
> > > > > ```<br>
> > > > > 2<br>
> > > > > / \<br>
> > > > > 4 5<br>
> > > > > | / \<br>
> > > > > | 7 8<br>
> > > > > \ /____/<br>
> > > > > 9 --- loop ---> 2<br>
> > > > > |<br>
> > > > > exit<br>
> > > > > ```<br>
> > > > > Is this the simplest loop where ld/st promotion applies?<br>
> > > > 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'.<br>
> > > Could you add a check-not for further uses of __profc*?<br>
> > The purpose is to test multiple counters hoisted<br>
> Done.<br>
</div></div>I think this is still missing<br>
<br>
<br>
<a href="https://reviews.llvm.org/D34085" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34085</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>