[PATCH] D86970: [IRSim] Adding IRSimilarityCandidate that contains a region of IRInstructionData

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:200
+    // have an unsigned integer assigned to it.
+    SmallVector<Value *, 4> Args = ID->OperVals;
+    for (Value *Arg : Args)
----------------
I think you can avoid the expensive vector copy by writing this as:

`for (Value *Arg : ID->OperVals) {`


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:211
+    if (ValueToNumber.find(ID->Inst) == ValueToNumber.end()) {
+      ValueToNumber.insert(std::make_pair(ID->Inst, LocalValNumber));
+      NumberToValue.insert(std::make_pair(LocalValNumber, ID->Inst));
----------------
Could do:

`ValueToNumber.emplace_back(ID->Inst, LocalValNumber);`


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:236
+        IRInstructionData &A = std::get<0>(R);
+        IRInstructionData &B = std::get<0>(R);
+        if (!A.Legal || !B.Legal)
----------------
A and B point at the same thing. Maybe you meant std::get<1>(R) for the latter?


================
Comment at: llvm/lib/Analysis/IRSimilarityIdentifier.cpp:239
+          return false;
+        return isClose(std::get<0>(R), std::get<1>(R));
+      });
----------------
`isClose(A, B)`, since there are names for them already?


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

https://reviews.llvm.org/D86970