[PATCH] D145171: [FSAFDO] Improve FS discriminator encoding

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

Thanks for the change! I'm going to start the integration with pseudo probe from here.



================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:64
   uint64_t Ret = updateHash(std::to_string(DIL->getLine()));
   Ret ^= updateHash(BB.getName());
   Ret ^= updateHash(DIL->getScope()->getSubprogram()->getLinkageName());
----------------
BB could have different names between the release and the debug compiler, or using or not using `-fdiscard-value-names`. This may lead to non-determinism in computing FS discriminators. Maybe consider using an integer id for BB? It doesn't exist today though.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:149
       if (LineNo == 0)
         continue;
       unsigned Discriminator = DIL->getDiscriminator();
----------------
Should zero line number also get a discriminator? This may result in invalid line offsets with which all such instructions will end up sharing the same sample.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:155
+
+      LocationDiscriminator LD{DIL->getFilename(), LineNo, Discriminator,
+                               CallStackHashVal};
----------------
Wondering if `DIL->getFilename()` is still needed since `CallStackHashVal` includes the caller linkage names which should help avoid hash conflicts.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:166
+      if (ImprovedFSDiscriminator)
+        DiscriminatorCurrPass += BBSizeHash;
+      else
----------------
Wondering if `BBSizeHash` is stable enough run-to-run. If consecutive builds have slight change in block size, their discriminators may not match?

Including callsite hash `LocationDiscriminator` sounds a great improvement. I'm wondering how helpful it is to include `BBSizeHash` in the discriminator encoding. Have you evaluated the two changes separately?


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

https://reviews.llvm.org/D145171