<div dir="ltr">We have only 64 bit in the hash. There could be some collisions. We were thinking they are only the corners cases.<div><br></div><div>I discussed with David Li about the potentially increased chance of collisions for this change. We were thinking to expand the hash, but that would need some non-trivial change.</div><div>The hash needs to be expanded anyway because currently we only encode indirect-call counters in the hash and leave out other value-profile counters.</div><div><br></div><div>For this case, I think we need to improve the code to handle the already happened collision. We could print an warning and result instead of assert.</div><div><br></div><div>I will send a patch to this.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Apr 1, 2019 at 9:00 PM Peter Collingbourne via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">pcc added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:673<br>
+  // Reserve bit 60-63 for other information purpose.<br>
+  FunctionHash &= 0x0FFFFFFFFFFFFFFF;<br>
+  if (IsCS)<br>
----------------<br>
Hi Rong, I discovered a problem with this line of code. If the number of select instructions when the profile was collected differs by a multiple of 16 from the number when the profile is used, we get a hash collision which results in us hitting an assertion here:<br>
<a href="http://llvm-cs.pcc.me.uk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#1042" rel="noreferrer" target="_blank">http://llvm-cs.pcc.me.uk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp#1042</a><br>
<br>
Granted, the same problem seems to have already existed for differences that are multiples of 256, but this seems to make the problem more likely to occur.<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D54175/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54175/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D54175" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54175</a><br>
<br>
<br>
<br>
</blockquote></div>