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

[Rong Xu via Phabricator via llvm-commits]

xur added a comment.

Thanks Hongtao for the reviews and comments. I'll update the patch shortly.



================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:310
 ErrorOr<uint64_t>
 SampleProfileLoaderBaseImpl<BT>::getInstWeightImpl(const InstructionT &Inst) {
   const FunctionSamples *FS = findFunctionSamples(Inst);
----------------
hoy wrote:
> hoy wrote:
> > 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);
> >   }
> Or maybe just override the existing virtual function `getInstWeight`, just like `SampleProfileLoader::getInstWeight`
I can do that. This way we don't need shouldIgnoreInst() function.

I have this shouldIgnoreInst() because there are types of instructions in SamplePorfiles try to ignore too. I want to have a unify interface for that. But this seems to be an overkill. 




================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:82
+    Ret ^= updateHash(std::to_string(DIL->getLine()));
+    Ret ^= updateHash(DIL->getScope()->getSubprogram()->getLinkageName());
+  }
----------------
hoy wrote:
> `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`?
Good to know this. Have see seen cases that empty string leads to hash conflicts?
Will getName() also return empty string?

We can add another interface in DISubprogram? in DebuginfoMetadata.h?




================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:113
+  if (ImprovedFSDiscriminator)
+    LowBitTemp -= 1;
+  unsigned BitMaskBefore = getN1Bits(LowBitTemp);
----------------
hoy wrote:
> 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?
This is a bug fix. I notice that issue for a while but I did fix it because it changes of some discriminators.
I will add an assert here.


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

https://reviews.llvm.org/D145171