<div dir="ltr">Also, the InstrProfRecordWithKey can inherit from InstProfRecord ...<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 5, 2017 at 1:54 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Jul 5, 2017 at 1:27 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Thu, Jun 29, 2017 at 7:48 PM David Li via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davidxl added a comment.<br>
<br>
In summary, I think this patch should be minimized by<br></blockquote></span><div><br>While I appreciate/value minimizing churn - I'm mostly motivated by/trying to ensure the best form for the code, rather than the smallest change here.<br></div></div></div></blockquote><div><br></div><div><br></div></span><div>It is not just about minimizing the churns. Currently the class InstrProfRecord abstracts everything the client needs to know about the per-function profile which is nice thing we'd like to maintain. </div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>If they happen to coincide, that's great.<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">1. sinking the minimal number of interfaces into Counter subclass<br>
2. for any sunk interface, provide a wrapper in the parent class InstrProfRecord<br>
3. avoid exposing internal members to client side other than InstrProfWriters (may be making those method private and making Writer a friend ?)<br></blockquote></span><div><br>I did consider this (& certainly open to discussing the tradeoff further to see if it's the right one) - my thinking was more that this change indicates a new use case/way to pivot the InstrProfRecord data structure that's not exclusively in the domain of the InstrProfWriter.<br><br>Looking at it that way, it seems to me that it's a good thing to move more parts of the code over to only caring about the part of the data structure that's relevant to them - which in many cases is just the counters (though I'd love a better name that "CounterHolder" (& could make it a non-nested class, especially if it had a good standalone name) - even potentially using the InstrProfRecord as the name for this name/hash-less part of the data and "NamedInstrProfRecord" for the Name+Hash+InstrProfRecord - though I haven't prototyped that/given it a lot of thought)<br></div></div></div></blockquote><div><br></div></span><div>I like the suggestion to promote the CounterHolder class into the first class citizen -- InstrProfRecord with the existing InstrProfRecord be renamed to NamedInstrProfRecord or InstrProfRecordWithKey. I expect majority of the APIs do not care about the name key anyway. The only affected interfaces are related to profile reader iterators and a few writer changes.</div><div><br></div><div>I think the above will be clean.</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><div><div class="m_6009155048529384991m_8869582409969383960h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
4. split out unrelated changes<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>Prof.h:253<br>
+ ArrayRef<InstrProfValueData> VDs, uint64_t Sum,<br>
+ InstrProfValueKind ValueKind, uint32_t MaxMDCount);<br>
<br>
----------------<br>
unrelated format change?<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>Prof.h:701<br>
+ /// counts of all target values at this site.<br>
+ inline uint64_t getValueForSite(InstrProfValue<wbr>Data Dest[],<br>
+ uint32_t ValueKind, uint32_t Site) const;<br>
----------------<br>
format change?<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>ProfData.inc:317<br>
*/<br>
- void deserializeTo(InstrProfRecord &Record,<br>
- InstrProfRecord::ValueMapType *VMap);<br>
+ void deserializeTo(InstrProfRecord:<wbr>:CounterHolder &Record,<br>
+ InstrProfRecord::CounterHolde<wbr>r::ValueMapType *VMap);<br>
----------------<br>
Why this change? IMO, we should reduce exposing CounterHolder if possible and maintain/use APIs at the InstrProfRecord level.<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>ProfData.inc:361<br>
*/<br>
- static uint32_t getSize(const InstrProfRecord &Record);<br>
+ static uint32_t getSize(const InstrProfRecord::CounterHolder &Counters);<br>
/*!<br>
----------------<br>
Same here.<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>ProfData.inc:530<br>
INSTR_PROF_VISIBILITY uint32_t<br>
-getValueProfDataSize(ValuePro<wbr>fRecordClosure *Closure) {<br>
+getValueProfDataSize(const void *Record, const ValueProfRecordVTable *VTable) {<br>
uint32_t Kind;<br>
----------------<br>
Can this be split into different patch?<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/Instr<wbr>ProfWriter.h:66<br>
/// Write \c Record in text format to \c OS<br>
- static void writeRecordInText(const InstrProfRecord &Record,<br>
+ static void writeRecordInText(StringRef Name, uint64_t Hash,<br>
+ const InstrProfRecord::CounterHolder &Counters,<br>
----------------<br>
There is no need to change this interface. You are passing name, hash etc, so might as well create a InstrProfRecord.<br></blockquote></div></div><div><br>But then the counters must be copied or moved into the InstrProfRecord, making it destructive (or expensive if it's a copy instead of a move) to the writing step (not the end of the world for InstrProfWriter - but seems unnecessarily shuffling things around when this is an implementation detail of the writer anyway).<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProf.cpp:<wbr>641<br>
+static const ValueProfRecordVTable InstrProfRecordCountersVTable = {<br>
+ getNumValueKindsInstrProfCount<wbr>ers,<br>
+ getNumValueSitesInstrProfCount<wbr>ers,<br>
----------------<br>
These naming changes are not necessary<br></blockquote></span><div><br>Seemed relevant if the name of the data structure being operated on changes (but I agree, if the design ends up being not to reduce the surface area of code that only needs to handle the counters, then these names wouldn't need to change).<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProf.cpp:<wbr>651<br>
+uint32_t<br>
+ValueProfData::getSize(const InstrProfRecord::CounterHolder &Counters) {<br>
+ return getValueProfDataSize(&Counters<wbr>, &InstrProfRecordCountersVTable<wbr>);<br>
----------------<br>
The interface change here is not necessary.<br>
<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProfWrite<wbr>r.cpp:152<br>
+ .second; // FIXME: Avoid copying by fixing the addRecord API<br>
+ SummaryBuilder->addRecord(std:<wbr>:move(R));<br>
<br>
----------------<br>
davidxl wrote:<br>
> Summary Builder does not need name, hash etc. so perfhaps fixing API is the right way.<br>
On second thought, to reduce exposing implementation details and minimizing interface changes, it is cleaner to just do<br>
<br>
SummaryBuilder->addRecord(Inst<wbr>rProfRecord(K, ProfileData.first, Counters));<br></blockquote></span><div><br>But the SummaryBuilder doesn't need/use the name or hash - so it seems strange to me to pass that. (again, to the design tradeoff/discussion above)<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Similarly for the SerializeFrom call below and ValueProfData::getSize() call above.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D34838" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3483<wbr>8</a><br>
<br>
<br>
<br>
</blockquote></span></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>