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

[Rong Xu via Phabricator via llvm-commits]

xur marked an inline comment as done.
xur added a comment.

Thanks to Hongtao and Weilei for the review!



================
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};
+
----------------
wenlei wrote:
> Not a big deal but do we actually need a map here? Could just use `getBaseDiscriminatorBits + i * FSDiscriminatorWidth` in a helper? 
Yes. I think we can do that. The reason we have a map for the bits is because I used variable length of bit for different FS discriminator passes in my earlier implementation /experiments. 
I think we will end up with using fixed length here. I'll change this.


================
Comment at: llvm/include/llvm/Support/Discriminator.h:64
+static inline unsigned getFSPassBitBegin(unsigned I) {
+  if (I == 0)
+    return 0;
----------------
hoy wrote:
> 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()` ?
This part is changed to go with Wenlei's suggestion.
I will assert that I >0 and I<=0 getNumFSPasses(). This way we don't need the check. We don't have a caller with 0 as the argument anyway.


================
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() {
----------------
wenlei wrote:
> There're mentions of FS passes and FD passes in multiple places in the comment, are they referring to the same thing?  
Sorry. FD is a typo. Fixed.


================
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()));
----------------
wenlei wrote:
> A comment explaining the purpose of final discriminator pass would be helpful here. 
Will do.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:312
+      if (ProfileIsFS)
+        Discriminator &= getDiscriminatorMask();
+
----------------
hoy wrote:
> 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?
I'm not sure if I understand the question.
Hers the "discriminator" is the one from profile which can have all the discriminator bits (i.e. including the later passes).  Here we use the mask to remove the later bits and merge them together.


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


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

https://reviews.llvm.org/D103041