[PATCH] D136211: [llvm-profdata] Check for all duplicate entries in MemOpSize table

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: llvm/lib/ProfileData/InstrProfWriter.cpp:535
+      for (uint32_t I = 0; I < ND; I++) {
+        if ((VK != IPVK_IndirectCallTarget) && SeenValues.count(VD[I].Value)) {
+          return make_error<InstrProfError>(instrprof_error::invalid_prof);
----------------
Oh I missed this the first time.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp:320
+    if (SeenSizeId.count(V)) {
       LLVM_DEBUG(dbgs() << "Invalid Profile Data in Function " << Func.getName()
+                        << ": Two identical values in MemOp value counts.\n");
----------------
ormris wrote:
> ellis wrote:
> > Why not make this a full diagnostic error/warning? Is this something a user could encounter in the wild?
> > Why not make this a full diagnostic error/warning?
> 
> Well, my understanding is that invalid profile data is usually skipped with a diagnostic. I don't want this error to be out of line with other profile data diagnostics. If something more severe is usually emitted, then we should change this error as well.
> 
> > Is this something a user could encounter in the wild?
> 
> This does occur in the wild, though it's very rare. We originally discovered this issue when a user sent us invalid bitcode which we determined had been generated by this pass.
> This does occur in the wild, though it's very rare. We originally discovered this issue when a user sent us invalid bitcode which we determined had been generated by this pass.
If the compiler generated invalid bitcode then IMO we should definitely emit an error. Or maybe we can turn this into an assert at least? Maybe there is something I'm missing because I'm not familiar with MemOP.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136211/new/

https://reviews.llvm.org/D136211