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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/FSAFDODiscriminator.h:11
+// discriminators to the instruction debug information.  With this, a cloned
+// machine instruction in a different MachineBasicBlock will have it's own
+// discriminator value.  This is done in a AddFSDiscriminators pass.
----------------
Nit: its


================
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) {
----------------
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?


================
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());
----------------
`BB.getName()` and `getSubprogram()->getLinkageName()` could be empty. Could it be a problem?


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


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:260
+
+  ProfileIsFS = ProfileIsFSDisciminator;
   for (; !LineIt.is_at_eof(); ++LineIt) {
----------------
Set `FunctionSamples::ProfileIsFS` as well to be consistent with the extbin reader?


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

https://reviews.llvm.org/D102246