[PATCH] D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO

[Rong Xu via Phabricator via llvm-commits]

xur marked 2 inline comments as done.
xur added a comment.

Hoy, Thanks for comments!



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:1771
+  /// (i.e. zero out the B-th and above bits for D (B is 0-based and inclusive).
+  // Example: an input of (0x1FF, 7) returns 0xFF.
+  static unsigned getMaskedDiscriminator(unsigned D, unsigned B) {
----------------
hoy wrote:
> xur wrote:
> > wmi wrote:
> > > If the first bit is the 0th bit, the 1 in 0x1FF is in the 8th bit and why input B = 7 can clear it?
> > The comment is not accurate. It should be "Zero out the (B+1)-th and above bits for D. 
> Looks like B == 0 is a special case, can you please update the comment for that too?
Thanks. Will update this.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:47
+  uint64_t Ret = MD5Hash(std::to_string(DIL->getLine()));
+  Ret ^= MD5Hash(BB.getName());
+  Ret ^= MD5Hash(DIL->getScope()->getSubprogram()->getLinkageName());
----------------
hoy wrote:
> `BB.getName()` and `getSubprogram()->getLinkageName()` could be empty. Could it be a problem?
MD5Hash can take empty string. I did not encounter issues for this.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:77
+  // Mask of discriminators for bits specific to this pass.
+  unsigned BitMaskThisPass = BitMaskNow ^ BitMaskBefore;
+  unsigned NumNewD = 0;
----------------
hoy wrote:
> The xor could result in 1 for higher bits above HighBit. Does it matter?
This should not happen. The bits higher than HighBit should be 0 for for variables. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:260
+
+  ProfileIsFS = ProfileIsFSDisciminator;
   for (; !LineIt.is_at_eof(); ++LineIt) {
----------------
hoy wrote:
> Set `FunctionSamples::ProfileIsFS` as well to be consistent with the extbin reader?
Extbin format is a little different from other formats. We assume the format is encoded in summary section.
It's in function SampleProfileReaderExtBinaryBase::readOneSection(). 

I did not overwrite the flag in summary section with the internal flag -- this flag is mostly used in test and debug with text and binary format. I assume most users will be using extbinary format where flag variable in summary section should have been set properly.


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

https://reviews.llvm.org/D102246