[PATCH] D76255: [SampleFDO] Port MD5 name table support to extbinary format.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Great to MD5 support added to ext-bin, thanks @wmi. After this change it seems compact-bin is completely superseded by ext-bin, do you plan to deprecate/remove it?



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:157
 
-enum SecFlags { SecFlagInValid = 0, SecFlagCompress = (1 << 0) };
+enum SecFlags { SecFlagInValid = 0, SecFlagCompress = (1 << 0), SecFlagMD5Name = (1 << 1) };
 
----------------
I thought `SecFlags` is a set of general purpose flags that can be applied to all section, at least `SecFlagCompress` falls under that category. For Name vs MD5Name, to some extended it's like section with different content. So would it make sense to have `SecMD5NameTable` in addition to `SecNameTable` for `SecType`, instead of relying on `SecFlags`? All reader/writer code can still be shared between the two.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:474
+          static_cast<sampleprof::SampleProfileWriterExtBinary *>(&Writer);
+      ExtBinaryWriter->setUseMD5();
+    }
----------------
The incompatibility between `-use-md5` and format is detected and handled at runtime. Can we do the same for `setUseMD5`? Specifically, make `setUseMD5` part of `SampleProfileWriter`, and error/assert in the base version of it - that would avoid the casting and simplifies a bit in couple places. (Could probably do the same for compression).



Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255