[PATCH] D147651: [PseudoProbe] Encode/Decode FS discriminator

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm with a nit, thanks.



================
Comment at: llvm/include/llvm/IR/PseudoProbe.h:95
 
+static inline bool HasDiscriminator(uint32_t Flags) {
+  return Flags & (uint32_t)PseudoProbeAttributes::HasDiscriminator;
----------------
nit: these helper function seem to start with lower case initial?


================
Comment at: llvm/include/llvm/IR/PseudoProbe.h:34
+  Sentinel = 0x2,           // A place holder for split function entry address.
+  HasFSDiscriminator = 0x4, // for probes with a FS discriminator
 };
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > At this layer, there's nothing specific to FS. I think we can just call it HasDiscriminator. This is essentially discriminator to probe, and we just currently use it for FSDiscriminator. 
> > > > 
> > > > Then s/FSDiscriminator/Discriminator/g for this entire patch. 
> > > I initially used "Discriminator" but switched to `FSDiscriminator` since pseudo probe itself has a unique id and shouldn't need the discriminator concept to be distinguished. WDYT? I don't have a strong preference though.
> > > but switched to FSDiscriminator since pseudo probe itself has a unique id and shouldn't need the discriminator concept
> > 
> > I don't understand the reasoning - the fact that we are now using FSDiscriminator contradicts that statement. FSDiscriminator is one kind of discriminator. 
> > 
> > I think it's perhaps better to just call it discriminator at this layer.
> > 
> > 
> Discriminator sounds a bit general to me, and I'm not sure if it can cause any confusion like one may question why it is needed since probe id is always unique. Maybe it was just me. If you just look at the encoding layer one may not get a full picture that probe id is unique in the first place and having a general discriminator may make sense. On the other hand, FSDiscriminator is very specific and clear. 
> 
> 
I see that you want to use "FSDiscriminator" to remind people that it's only for FS. But we have to accept that FSDiscriminator one kind of a Discriminator. Probe id is unique when it's assigned, but it's no longer unique after duplication, which is where discriminator or FSDiscriminator comes in. Anyways, thanks for making the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147651