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

[Wenlei He via Phabricator via llvm-commits]

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




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


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5854-5856
   int64_t Type;
   int64_t Attr;
+  int64_t FSDiscriminator;
----------------
any reasons for these field to all use int64? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147651