<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 29, 2017 at 7:48 PM David Li via Phabricator <<a href="mailto:reviews@reviews.llvm.org">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><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><br>If they happen to coincide, that's great.<br> </div><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><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><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/InstrProf.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/InstrProf.h:701<br>
+    /// counts of all target values at this site.<br>
+    inline uint64_t getValueForSite(InstrProfValueData Dest[],<br>
+                                    uint32_t ValueKind, uint32_t Site) const;<br>
----------------<br>
format change?<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProfData.inc:317<br>
    */<br>
-  void deserializeTo(InstrProfRecord &Record,<br>
-                     InstrProfRecord::ValueMapType *VMap);<br>
+  void deserializeTo(InstrProfRecord::CounterHolder &Record,<br>
+                     InstrProfRecord::CounterHolder::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/InstrProfData.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/InstrProfData.inc:530<br>
 INSTR_PROF_VISIBILITY uint32_t<br>
-getValueProfDataSize(ValueProfRecordClosure *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/InstrProfWriter.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><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><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:641<br>
+static const ValueProfRecordVTable InstrProfRecordCountersVTable = {<br>
+    getNumValueKindsInstrProfCounters,<br>
+    getNumValueSitesInstrProfCounters,<br>
----------------<br>
These naming changes are not necessary<br></blockquote><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><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:651<br>
+uint32_t<br>
+ValueProfData::getSize(const InstrProfRecord::CounterHolder &Counters) {<br>
+  return getValueProfDataSize(&Counters, &InstrProfRecordCountersVTable);<br>
----------------<br>
The interface change here is not necessary.<br>
<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProfWriter.cpp:152<br>
+              .second; // FIXME: Avoid copying by fixing the addRecord API<br>
+      SummaryBuilder->addRecord(std::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(InstrProfRecord(K, ProfileData.first, Counters));<br></blockquote><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><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/D34838</a><br>
<br>
<br>
<br>
</blockquote></div></div>