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

[Rong Xu via Phabricator via llvm-commits]

xur marked 4 inline comments as done.
xur added inline comments.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:2246
 Optional<const DILocation *> DILocation::cloneWithBaseDiscriminator(unsigned D) const {
   unsigned BD, DF, CI;
+  if (EnableFSDiscriminator)
----------------
wmi wrote:
> 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 ...;
> }
> ...
> ```
agreed. I will change this.


================
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 *>;
----------------
wmi wrote:
> How about using tuple for LocationDiscriminator?
Yes. I think that's a better choice.


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

https://reviews.llvm.org/D102246