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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:310
 ErrorOr<uint64_t>
 SampleProfileLoaderBaseImpl<BT>::getInstWeightImpl(const InstructionT &Inst) {
   const FunctionSamples *FS = findFunctionSamples(Inst);
----------------
How about making this function virtual and override it in MIR sample loader? Something like


	
  ErrorOr<uint64_t> getInstWeightImpl(const MachineInstr &Inst) {
    if (ImprovedFSDiscriminator && Inst.isMetaInstruction())
      return std::error_code();
    return SampleProfileLoaderBaseImpl<MachineBasicBlock>::getInstWeightImpl(
        Inst);
  }


================
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());
----------------
xur wrote:
> 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.
I see. It's only used in the old version.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:82
+    Ret ^= updateHash(std::to_string(DIL->getLine()));
+    Ret ^= updateHash(DIL->getScope()->getSubprogram()->getLinkageName());
+  }
----------------
`getLinkageName` may return an empty string C functions. We often use a trick to work it around:

    // Use linkage name for C++ if possible.
    auto Name = SP->getLinkageName();
    if (Name.empty())
      Name = SP->getName(); 

Actually there is a similar function `getCallStackHash` in `SampleProfileProbe.cpp`. Do you think it's possible to unify them and place it in a command file like `DebubInfo.h`?


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:113
+  if (ImprovedFSDiscriminator)
+    LowBitTemp -= 1;
+  unsigned BitMaskBefore = getN1Bits(LowBitTemp);
----------------
Is this fixing a bug of the old encoding?

BTW, should we assert the current pass is always not the base pass or LowBit is never zero?


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:149
       if (LineNo == 0)
         continue;
       unsigned Discriminator = DIL->getDiscriminator();
----------------
xur wrote:
> 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. 
Good to know you are working on a fix.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:155
+
+      LocationDiscriminator LD{DIL->getFilename(), LineNo, Discriminator,
+                               CallStackHashVal};
----------------
xur wrote:
> 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.
Thanks for pointing it out.


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

https://reviews.llvm.org/D145171