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

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: llvm/lib/CodeGen/MIRFSDiscriminator.cpp:67
+  using BBSet = DenseSet<const MachineBasicBlock *>;
+  using LocationDiscriminatorBBMap = DenseMap<LocationDiscriminator, BBSet>;
+  using LocationDiscriminatorCurrPassMap =
----------------
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.

 


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1181
+  if (EnableFSDiscriminator && !FSNoFinalDiscrim)
+    addPass(createMIRAddFSDiscriminatorsPass(PASS_LAST_DIS_BIT_BEG,
+                                             PASS_LAST_DIS_BIT_END));
----------------
hoy wrote:
> Wondering why using `PASS_LAST_DIS_BIT_BEG` here. Will other bits be used in later patches?
> 
This is to catch all the clones in the optimization pipeline, so we could sum the samples instead of using max.  We will have a FS profile loader here.  For earlier pass, there will be an FS Profile loader for each AddFSDiscriminatorPass.


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

https://reviews.llvm.org/D102246