[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);
----------------
xur wrote:
> 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. 
> 
> 
Thanks.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:82
+    Ret ^= updateHash(std::to_string(DIL->getLine()));
+    Ret ^= updateHash(DIL->getScope()->getSubprogram()->getLinkageName());
+  }
----------------
xur wrote:
> 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?
> 
> 
I haven't taken a deep look into whether empty strings can lead to hash conflicts for this case actually.

`getName() ` will always return the original demangled function name, and for C, it is just the linkage name.


A new interface in DebuginfoMetadata.h sounds good. Perhaps a new member function for `DILocation`?


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:166
+      if (ImprovedFSDiscriminator)
+        DiscriminatorCurrPass += BBSizeHash;
+      else
----------------
xur wrote:
> 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. 
> This seems to be better than applying wrong BB weights.

This makes sense.

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

Yeah, curious to see how impactful it is. Thanks.






================
Comment at: llvm/test/CodeGen/X86/fsafdo_test1.ll:5
 ; Check that fs-afdo discriminators are generated.
 ; CHECK: .loc    1 7 3 is_stmt 0 discriminator 2 # foo.c:7:3
 ; ChECK: .loc    1 9 5 is_stmt 1 discriminator 2 # foo.c:9:5
----------------
I have a question about keeping the original discriminator, i.e, 2 here. IIUC, the MIR sample loader will skip loading samples for the instruction. Do you think it should get a new discriminator so that it can use pass specific counters too? Let me know if I miss anything.


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

https://reviews.llvm.org/D145171