[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:
> 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.


================
Comment at: llvm/include/llvm/MC/MCPseudoProbe.h:114
   uint64_t Index;
+  uint64_t FSDiscriminator;
   uint8_t Attributes;
----------------
wenlei wrote:
> can we use 32bit here? 
Yes, 32-bit should be enough for now.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2349
+  OS << "\t.pseudoprobe\t" << Guid << " " << Index << " " << Type << " " << Attr
+     << " " << FSDiscriminator;
   // Emit inline stack like
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147651