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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/Support/Discriminator.h:55
+// Define the High bit for each FS pass.
+const unsigned char FSPassHighBit[] = {7, 13, 19, 25, 31};
+
----------------
Can we use unsigned int instead so no implicit conversion when FSPassHighBit is used? And how about using std::array so we can use FSPassHighBit.at(i) in FSPassBitBegin and FSPassBitEnd which will have array bounds check.


================
Comment at: llvm/include/llvm/Support/Discriminator.h:91-93
+  if (N == 31)
     return 0xFFFFFFFF;
   return (1 << (N + 1)) - 1;
----------------
Nit: For N==31, (1U << (N + 1)) - 1 is 0xFFFFFFFF so no if (N == 31) is needed.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:312-314
+        uint32_t MaskedDiscriminator = Discriminator;
+        MaskedDiscriminator &= getDiscriminatorMask();
+        Discriminator = MaskedDiscriminator;
----------------
Nit: Discrriminator &= getDiscriminatorMask();


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:529-530
+    if (ProfileIsFS) {
+      uint32_t MaskedDiscriminator = DiscriminatorVal & getDiscriminatorMask();
+      DiscriminatorVal = MaskedDiscriminator;
+    }
----------------
Nit: DiscrriminatorVal &= getDiscriminatorMask();


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:570-571
+    if (ProfileIsFS) {
+      uint32_t MaskedDiscriminator = DiscriminatorVal & getDiscriminatorMask();
+      DiscriminatorVal = MaskedDiscriminator;
+    }
----------------
Nit: DiscrriminatorVal &= getDiscriminatorMask();


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

https://reviews.llvm.org/D103041