[PATCH] D26420: Encode duplication factor from loop vectorization and loop unrolling to discriminator.

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
Comment at: lib/Transforms/Utils/AddDiscriminators.cpp:197
       // it in 1 byte ULEB128 representation.
-      unsigned Discriminator = (R.second ? ++LDM[L] : LDM[L]) & 0x7f;
+      unsigned Discriminator = ((R.second ? ++LDM[L] : LDM[L]) << 1) & 0x7f;
       I.setDebugLoc(DIL->cloneWithDiscriminator(Discriminator));
----------------
hfinkel wrote:
> Please add a utility function for the encoding.
Thanks for the comment!

I agree with all the comments, but before I address the comments and move forward with this encoding, let's discuss more whether we want to use lower bits to indicate discriminator type. Comparing with fixed position encoding (i.e. DP always consume 2 ULEB128 bytes):

Pros:
* saves ~1% debug line table size (on average)

Cons:
* added complexity to clang code and also offline profile creation code
* limited representation scope: with the new algorithm we need to have 1 extra bit to represent the profile type, which means we need to limit the useful bit to be 6 in order to fit it to 1-byte ULEB128. As a result, the maximum raw discriminator will be 63 instead of 127.

Comments?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:216
 
+extern cl::opt<bool> EncodeDuplicationInDiscriminators;
+
----------------
hfinkel wrote:
> Based on one of the other threads, I suppose we're going to add some command-line flag to enable/disable this? That being the case, we'll read this setting from some function attribute string instead of using  a cl::opt.
Yes, we will guard all the logic by check profile-debug flag once https://reviews.llvm.org/D25435 is submitted. I'm not sure whether it will be a function attribute string, or simply a flag passed down from frontend.

Meanwhile, I think it will be worth to have a flag to control whether we will encode duplication in discriminator so that we have a choice to fall back to the old discriminator assignment algorithm. We can remove the flag later if we find it useless.


https://reviews.llvm.org/D26420