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

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
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());
----------------
hoy wrote:
> 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.
You are exactly right! Note this is old version of Hash that will be deprecated. The new version will not use the name anymore.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:149
       if (LineNo == 0)
         continue;
       unsigned Discriminator = DIL->getDiscriminator();
----------------
hoy wrote:
> 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.
Zero line numbers do not get a discriminator. The number of zero line number instructions is actually pretty big. They can easily overflow the discriminators. We have another changes to deal with zero line number and instructions with empty DIL. They are still ongoing. 


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:155
+
+      LocationDiscriminator LD{DIL->getFilename(), LineNo, Discriminator,
+                               CallStackHashVal};
----------------
hoy wrote:
> Wondering if `DIL->getFilename()` is still needed since `CallStackHashVal` includes the caller linkage names which should help avoid hash conflicts.
CallStackHashValue is for DIL with getInlinedAt(). If this is not inlined instrution. CallStackHashValue returns 0.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:166
+      if (ImprovedFSDiscriminator)
+        DiscriminatorCurrPass += BBSizeHash;
+      else
----------------
hoy wrote:
> 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?
We have tried a few other ways for the Hash. It seems that it's better to be more strict on the match -- if it's mistach, we will not use the pass specific counters, but we still use the average count (branch portability) from previous rounds. This seems to be better than applying wrong BB weights.

I don't have the data for with and without BBSizeHash. I can a quick run to see if it has performance impact. 


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

https://reviews.llvm.org/D145171