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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


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


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:100
+      DiscriminatorCurrPass = DiscriminatorCurrPass << LowBit;
+      DiscriminatorCurrPass += getCallStackHash(BB, I, DIL);
+      DiscriminatorCurrPass &= BitMaskThisPass;
----------------
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? 


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

https://reviews.llvm.org/D102246