<div dir="ltr">This version looks good to me. Please make sure all tests are properly updated and watch out for test breakages from buildbots.<div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 11, 2015 at 6:33 PM, 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"><span class="">davidxl added inline comments.<br>
<br>
================<br>
</span>Comment at: lib/ProfileData/InstrProfReader.cpp:286<br>
@@ +285,3 @@<br>
+std::error_code RawInstrProfReader<IntPtrT>::readValueData(<br>
+    InstrProfRecord &Record) {<br>
+<br>
----------------<br>
This function will become only a couple of lines long if ValueProfileData is used as the format for Raw format too. Something like:<br>
<br>
<br>
support::endian::Endianess SrcEndianness = getDataEndianess(shouldSwapBytes, getHostEndianNess());<br>
.. VDataOrError = ValueProfData::getValueProfData(Data->Values, End, SrcEndianness);<br>
if (std::error_code EC = VDataOrError.getError())<br>
        return EC;<br>
VDataOrErr.get()->deserializeTo(Record, ...);<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:331<br>
@@ +330,3 @@<br>
<span class="">+                           ConstantExpr::getBitCast(Fn, Int8PtrTy) :<br>
+                           ConstantPointerNull::get(Int8PtrTy);<br>
</span>+<br>
----------------<br>
Move the whole expression into the .inc file for clarity. If it is too long, more this out of the function as an inline helper function and call the function in .inc file. The expression does not fit here (without context how it is used).<br>
<br>
inline Constant *getFuncAddr(..Fn) {<br>
}<br>
<br>
================<br>
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:335<br>
@@ +334,3 @@<br>
<span class="">+  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)<br>
</span>+    Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);<br>
+<br>
----------------<br>
Wrap the above and ConstantArray::get(Int16ArrayTy, Int16ArrayVals)) into one helper function, and reference the function call in .inc file.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D13308" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13308</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>