[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/FSAFDODiscriminator.cpp:90
+      unsigned Discriminator = DIL->getDiscriminator();
+      Discriminator &= BitMaskBefore;
+      LocationDiscriminator LD = {L, Discriminator};
----------------
wenlei wrote:
> xur wrote:
> > 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. 
> I was wondering whether `(Discriminator &= BitMaskBefore) == Discriminator` is expected here since we haven't set extra bits for current pass yet. If that's true, then the `&=` is not needed or could be an assert?
> 
> (I think somehow the comment got associated with the wrong line, it should about `Discriminator &= BitMaskBefore;`).
Yes. you are right here. I'll remove that stmt.


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:100
+      DiscriminatorCurrPass = DiscriminatorCurrPass << LowBit;
+      DiscriminatorCurrPass += getCallStackHash(BB, I, DIL);
+      DiscriminatorCurrPass &= BitMaskThisPass;
----------------
wenlei wrote:
> xur wrote:
> > 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. 
> Not a big deal, but I think since we always have line stack, and discriminator always has paired line stack, it doesn't need to have that info captured. Could saving the walk over inlineAt chain? 
when we insert the discriminator, we just just check the lineno (I meant the tuple of <filename, lineno, discriminator) -- this is not line stack. 
I did some pretty large build and did not find this part consume much time.

I'll keep this in mind and gonna revisit in the tuning.


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

https://reviews.llvm.org/D102246