[PATCH] D102246: [SampleFDO] New hierarchical discriminator for Flow Sensitive SampleFDO

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for splitting the patches.



================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2246
 Optional<const DILocation *> DILocation::cloneWithBaseDiscriminator(unsigned D) const {
   unsigned BD, DF, CI;
+  if (EnableFSDiscriminator)
----------------
It will be slightly clearer if the cases of EnableFSDiscriminator being true and false are not interleaved, so that it is easy to know that there won't be undefined use for DF and CI.
```
if (EnableFSDiscriminator) {
  return ...;
}
...
```


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:55
+
+bool AddFSDiscriminators::runOnMachineFunction(MachineFunction &MF) {
+  if (!EnableFSDiscriminator)
----------------
Could you describe how FSDiscriminator is computed here?


================
Comment at: llvm/lib/CodeGen/FSAFDODiscriminator.cpp:60-61
+  bool Changed = false;
+  using Location = std::pair<StringRef, unsigned>;
+  using LocationDiscriminator = std::pair<Location, unsigned>;
+  using BBSet = DenseSet<const MachineBasicBlock *>;
----------------
How about using tuple for LocationDiscriminator?


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:169
 
+// An option that disables inserting FS-AFDO discrmintators before emit.
+// This is mainly for debugging and tuning purpose.
----------------
typo: discriminators


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:173
+    FSNoFinalDiscrim("fs-no-final-discrim", cl::init(false), cl::Hidden,
+                     cl::desc("Do not insert FS-AFDO discrimnators before "
+                              "emit."));
----------------
typo: discriminators


================
Comment at: llvm/test/Transforms/SampleProfile/Inputs/fsafdo.prof:1
+work:33383580:1068858
+ 1: 981870
----------------
fsafdo.prof and fsafdo.extbinary.afdo may only be used in the other splitted patch.


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

https://reviews.llvm.org/D102246