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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:586
 
-  // These sets create a create a mapping between the values in one candidate
-  // to values in the other candidate.  If we create a set with one element,
-  // and that same element maps to the original element in the candidate
-  // we have a good mapping.
-  DenseMap<unsigned, DenseSet<unsigned>> ValueNumberMappingA;
-  DenseMap<unsigned, DenseSet<unsigned>> ValueNumberMappingB;
+  bool WasInserted;
+
----------------
I'd sink this closer to where it's used so there's less live range to think about.


================
Comment at: llvm/lib/Transforms/IPO/IROutliner.cpp:855
   // If the arguments are the same size, there are not values that need to be
-  // made argument, or different output registers to handle.  We can simply
-  // replace the called function in this case.
-  if (AggFunc->arg_size() == Call->arg_size()) {
+  // made argument, the arugment ordering has not been change, or different
+  // output registers to handle.  We can simply replace the called function in
----------------



================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:2015
+  // Check to make sure that we have a long enough region.
+  ASSERT_EQ(InstrList.size(), static_cast<unsigned>(6));
+  // Check that the instructions were added correctly to both vectors.
----------------
alternatively


================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:2017
+  // Check that the instructions were added correctly to both vectors.
+  ASSERT_TRUE(InstrList.size() == UnsignedVec.size());
+
----------------
`ASSERT_EQ(x, y);` will give better diagnostics when it fails than `ASSERT_TRUE(x == y);` does.


================
Comment at: llvm/unittests/Analysis/IRSimilarityIdentifierTest.cpp:2049
+    DenseSet<unsigned>::iterator It = P.second.find(Dest);
+    ASSERT_TRUE(It != P.second.end());
+  }
----------------
`ASSERT_NE`. Also, should it check what's in the set that was found?


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

https://reviews.llvm.org/D104143