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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:355
+  /// Set the bits for FS discriminators.
+  void setDiscriminatorMaskedBitFrom(uint32_t B) { MaskedBitFrom = B; }
+
----------------
This is an API that client of profile reader will need to call, so I'm wondering if we can make it higher level - i.e, instead of requiring the client to be aware and pass in the bit-wise mask, just let client set it for base discriminator (i == 0) or i-th FS discriminator? 


================
Comment at: llvm/include/llvm/Support/Discriminator.h:56
+// Define the High bit for each FS pass.
+const std::array<unsigned, 5> FSPassHighBit = {7, 13, 19, 25, 31};
+
----------------
Not a big deal but do we actually need a map here? Could just use `getBaseDiscriminatorBits + i * FSDiscriminatorWidth` in a helper? 


================
Comment at: llvm/include/llvm/Support/Discriminator.h:72
 
-#define PASS_1_DIS_BIT_BEG 8
-#define PASS_1_DIS_BIT_END 13
+// Return the beginning bit for the last FD pass.
+static inline int getLastFSPassBitBegin() {
----------------
There're mentions of FS passes and FD passes in multiple places in the comment, are they referring to the same thing?  


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:1181
   if (EnableFSDiscriminator && !FSNoFinalDiscrim)
-    addPass(createMIRAddFSDiscriminatorsPass(PASS_LAST_DIS_BIT_BEG,
-                                             PASS_LAST_DIS_BIT_END));
+    addPass(createMIRAddFSDiscriminatorsPass(getLastFSPassBitBegin(),
+                                             getLastFSPassBitEnd()));
----------------
A comment explaining the purpose of final discriminator pass would be helpful here. 


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:564
+    uint32_t DiscriminatorVal = *Discriminator;
+    if (ProfileIsFS)
+      DiscriminatorVal &= getDiscriminatorMask();
----------------
We could move the ProfileIsFS check into getDiscriminatorMask and make the mask all 1s for non-FS case? 


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

https://reviews.llvm.org/D103041