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

[Matthew Voss via Phabricator via llvm-commits]

ormris added inline comments.


================
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");
----------------
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.


================
Comment at: llvm/test/tools/llvm-profdata/invalid-profile-gen-dup-entries.proftext:4
+# IR level Instrumentation Flag
+# CHECK: warning: invalid profile created
+:ir
----------------
ellis wrote:
> It seems strange that this is emitted as a warning rather than an error. Is there a case where this warning can be safely ignored?
My original intention was to make this an error (see the earlier patch), but my experiments seem to indicate that very few users 1) encounter the bug that generates this invalid profile data and 2) have the correct conditions to force the MemOpSize pass to use that invalid profile data. If the 3 largest counts aren't invalid, it seems like generating invalid bitcode is unlikely. We also haven't seen many users report this diagnostic to us.

That said, while I'm comfortable leaving it as a warning, but I would still prefer this to be an error. If it's an error, we would likely get more data to reproduce this issue. @xur, do you have any thoughts on this?


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

https://reviews.llvm.org/D136211