[PATCH] D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO

Tue May 18 10:25:01 PDT 2021

xur added inline comments.

================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:67
+  using BBSet = DenseSet<const MachineBasicBlock *>;
+  using LocationDiscriminatorBBMap = DenseMap<LocationDiscriminator, BBSet>;
+  using LocationDiscriminatorCurrPassMap =
----------------
xur wrote:
> hoy wrote:
> > Nit: not sure if `std::unordered_multimap` can be a bit faster by not constructing a `BBSet` if most instructions are not duplicated.
> Do you mean line line 67 using 
> std::unordered_multimap<LocationDiscriminator, const MachineBasicBlock *> ?
> 
> I'm not sure. 
> My impression is DenseMap performs closely to unordered_multimap. You are considering the cost of constructing BBSet. I agree with your on this. But for some cases, there are lot instruction duplicattion for some <line, discriminator> -- like some codes in stl header. In these cases, unordered_multimap might not perform well as it needs to store all the BBs.
> 
> Let me run an experiments to compare. I will report back.
> 
>  
When I tried the multimap, I found it did not fit here well here.  The problem is for the same <file, lineno, discriminator> tuple, if they are in the same BB, BBset will aggregate them together. For the multimap, they are of different entries. Without checking all the entries with the same key, I could not tell if they are with the same BB, or with different BB.  In short, we need to check all the entries and find out (1) if they are from the same BB (2) if there are two distinct BBs in the map. The code is not as clean as using a set.

Anyway, I tested both implementations using SPEC2006/Xalancbmk and one large google benchmark. The compiler time for these two about the same (note that compiler time has a pretty large noise range).

I think I would like to keep DenseSet version.

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

https://reviews.llvm.org/D102246