[PATCH] D104143: [IRSim][IROutliner] Canonicalizing commutative value numbering between similarity sections.
Jessica Paquette via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 24 13:06:41 PDT 2021
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
More information about the llvm-commits
mailing list