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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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
 };
----------------
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. 




================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2349
+  OS << "\t.pseudoprobe\t" << Guid << " " << Index << " " << Type << " " << Attr
+     << " " << FSDiscriminator;
   // Emit inline stack like
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > should we skip if disc is 0, meaning no disc? 
> > This is for easy handling on the decoding side, which looks for this field unconditionally. The asm form of probes is mostly for debugging, and we haven't optimized for it.
> But I thought that is what the PseudoProbeAttributes is for - i.e. discriminator field only exists when attribute is set, and decoder is supposed to check attribute first before reading the field..
Yeah, the binary decoder currently does that. Makes sense for the asm decoder to also do that.


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5854-5856
   int64_t Type;
   int64_t Attr;
+  int64_t FSDiscriminator;
----------------
wenlei wrote:
> any reasons for these field to all use int64? 
because `parseIntToken` takes int64 as the parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147651