[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:100
+      DiscriminatorCurrPass = DiscriminatorCurrPass << LowBit;
+      DiscriminatorCurrPass += getCallStackHash(BB, I, DIL);
+      DiscriminatorCurrPass &= BitMaskThisPass;
----------------
wenlei wrote:
> xur wrote:
> > 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.
> ok, it's fine for now. For existing sample loader, we use nested FunctionSamples to get profile for instruction based on its line stack (`FunctionSamples::findFunctionSamples`), so the use of line stack is implicit there. Not sure if FS sample loader is different. 
> ok, it's fine for now. For existing sample loader, we use nested FunctionSamples to get profile for instruction based on its line stack (`FunctionSamples::findFunctionSamples`), so the use of line stack is implicit there. Not sure if FS sample loader is different. 

Oh. I see what you meant here now. FS sample loader uses the same mechanism. In this sense, yes, the location has the call stack info. I'll do some more experiments on this. For now, let's just keep it.


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

https://reviews.llvm.org/D102246