[PATCH] D86968: [IRSim] Adding IR Instruction Mapper

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:106
+  /// The types of the operands in the Instruction.
+  SmallVector<Type *, 4> OperTypes;
+  /// The legality of the wrapped instruction. This is informed by InstrType,
----------------
How are OperTypes different from OperVals getTypes() ? 


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:111
+  /// considered similar.
+  bool Legal = true;
+
----------------
Any reason Legal is true by default? Did I miss a comment here? 


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:118
+  /// Instruction performs the same operation.
+  IRInstructionData(Instruction &I, bool Legality);
+
----------------
Can `Instruction &I` be `const Instruction &I` ?


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:173
+    using llvm::hash_value;
+    assert(E && "IRInstructionData is a nullptr?");
+    return hash_value(*E);
----------------
Can you make these a pass by reference to avoid the need for assert? 


================
Comment at: llvm/include/llvm/Analysis/IRSimilarityIdentifier.h:183
+
+    assert(LHS && RHS && "nullptr should have been caught by getEmptyKey?");
+    return isClose(*LHS, *RHS);
----------------
Same, pass by reference? 


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

https://reviews.llvm.org/D86968