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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel 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));
----------------
danielcdh wrote:
> 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?
> saves ~1% debug line table size (on average)

Yes. but this 1% is 20% or more of the increase. More importantly for me, it reduces the relative expense (in terms of binary-size increase) of using distinct location tags vs. using duplication factors.

> added complexity to clang code and also offline profile creation code

If this patch is any indication, then the extra complexity seems minor.


================
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:
> danielcdh wrote:
> > 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?
> > saves ~1% debug line table size (on average)
> 
> Yes. but this 1% is 20% or more of the increase. More importantly for me, it reduces the relative expense (in terms of binary-size increase) of using distinct location tags vs. using duplication factors.
> 
> > added complexity to clang code and also offline profile creation code
> 
> If this patch is any indication, then the extra complexity seems minor.
> 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.

I don't like the fixed bit limit here. How about using the high bit to indicate that there are more bits in the discriminator (that's how ULEB128 itself works, right?)?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:216
 
+extern cl::opt<bool> EncodeDuplicationInDiscriminators;
+
----------------
danielcdh wrote:
> 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.
> 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.

I think it needs to be an attribute. Otherwise, it won't work correctly with LTO.



https://reviews.llvm.org/D26420