[PATCH] D104143: [IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:572
 
+  /// Create a mapping from the value numbering, to a different separate set of
+  /// numbers, this will serve as a guide for relating one candidate to another
----------------
break up run-on?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:579
+  /// numbering for.
+  static void createCanonicalFor(IRSimilarityCandidate &A);
+
----------------
- Use a more descriptive name than `A`?
- Maybe `createCanonicalMappingFor` would be a better name?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:581
+
+  /// Create a mapping for the value numbering of the calling
+  /// IRSimilarityCandidate, to a different separate set of numbers, based on
----------------
sentence is kind of long, break it up?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:594
+  void createCanonicalRelationFrom(
+      IRSimilarityCandidate &A,
+      DenseMap<unsigned, DenseSet<unsigned>> &ToAMapping,
----------------
more descriptive name than `A`?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:660
 
+  Optional<unsigned> getCanonicalNum(unsigned N) {
+    DenseMap<unsigned, unsigned>::iterator NCIt = NumberToCanonNum.find(N);
----------------
doxygen comment?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:667
+
+  Optional<unsigned> fromCanonicalNum(unsigned N) {
+    DenseMap<unsigned, unsigned>::iterator CNIt = CanonNumToNumber.find(N);
----------------
doxygen comment?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:759
+
+  assert(this->CanonNumToNumber.size() == 0 &&
+         "Canonical Relationship is non-empty");
----------------
do you need `this` here?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:802
+    unsigned CanonNum = *A.getCanonicalNum(ResultGVN);
+    this->CanonNumToNumber.insert(std::make_pair(CanonNum, SourceGVN));
+    this->NumberToCanonNum.insert(std::make_pair(SourceGVN, CanonNum));
----------------
do we need `this` here?


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

https://reviews.llvm.org/D104143