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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
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) };
 
----------------
wmi wrote:
> wenlei wrote:
> > 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.
> SecFlags was not designed to be general purpose only. I want to make it more flexible so that every section can define their own flags. I want to set aside some bits for general purpose only flags and some bits for  section specific flags. For section specific flag, every section can reuse the same bit for different purpose. We may need a verification in place to prevent misuse. If it sounds good to you, I will add some comments describing it and put the verification in place.   
Thanks for clarification - that sounds good to me, and yes some comments and verification would be useful.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:474
+          static_cast<sampleprof::SampleProfileWriterExtBinary *>(&Writer);
+      ExtBinaryWriter->setUseMD5();
+    }
----------------
wmi wrote:
> wenlei wrote:
> > 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).
> > 
> I was trying to find some balance here and not to put too much special things in the base class. For SampleProfileReader, if we don't add the interfaces in it, we will have many places that we need the casting, so it has a bunch of special interfaces there. For SampleProfileWriter, we only have a few places needing the special interfaces so adding the casting is still ok. I can change the code and only add casting once here. 
> 
> I am just not quite sure which way is better. If you feel adding the interfaces in SampleProfileWriter is better, I will be happy to do it. 
> 
>  
Conceptually, MD5 name doesn't have to be tied to extbinary, e.g. we could potentially have MD5 as name even for text profile. So maybe it's indeed general enough that warrant it being in base class, it's just we only support that for extbinary today. That said, I don't have a strong opinion either..


Repository:
  rL LLVM

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

https://reviews.llvm.org/D76255