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

[Rong Xu via Phabricator via llvm-commits]

xur marked 3 inline comments as done.
xur added inline comments.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:90
+      unsigned Discriminator = DIL->getDiscriminator();
+      Discriminator &= BitMaskBefore;
+      LocationDiscriminator LD = {L, Discriminator};
----------------
wenlei wrote:
> When discriminator passes run in order, do we actually expect anything to add bits outside of  BitMaskBefore?
I'm not fully understand the question. 

This is to compute the bits that can be set in this pass.  BitMaskBefore for the mask for the bits that have been set by previous passes -- it assumes the the previous discriminators only touch the lower bits. 


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:100
+      DiscriminatorCurrPass = DiscriminatorCurrPass << LowBit;
+      DiscriminatorCurrPass += getCallStackHash(BB, I, DIL);
+      DiscriminatorCurrPass &= BitMaskThisPass;
----------------
wenlei wrote:
> This seems different from the description in original RFC where you talked about using count of total discriminators and sequential id as seed for a fs-discriminator. Given that we still use inline stack as part of line info, and discriminator is on top of line stack, what extra benefit does hashing the entire inline stack bring us? 
yes. this part has gone through quite some change before and after RFC. Here we still uses a sequential id. 
Add a callstack hash is just one more safety measure to catch the mismatches. 


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

https://reviews.llvm.org/D102246