[PATCH] D103041: [SampleFDO] New hierarchical discriminator for FS SampleFDO (ProfileData part)

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/Support/Discriminator.h:64
+static inline unsigned getFSPassBitBegin(unsigned I) {
+  if (I == 0)
+    return 0;
----------------
Does it make sense to include 0 in the `FSPassHighBit` array so that the check can be omitted?

How about add a assert of `I <= getNumFSPasses()` ?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:312
+      if (ProfileIsFS)
+        Discriminator &= getDiscriminatorMask();
+
----------------
Assuming the sample loader runs in multiple places of the pipeline, does it make sense to assert the length of `Discriminator` matches the length of the mask? IIUC, if the number of valid bits of `Discriminator` is more than that of the mask, it means we are loading the profile of an earlier stage and we should use a longer mask?


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

https://reviews.llvm.org/D103041