Address comments from ellishg

Author: kyulee-com

llvm/lib/IR/StructuralHash.cpp

@@ -69,20 +69,13 @@ class StructuralHashImpl {
   stable_hash hashAPInt(const APInt &I) {
     SmallVector<stable_hash> Hashes;
     Hashes.emplace_back(I.getBitWidth());
-    for (unsigned J = 0; J < I.getNumWords(); ++J)
-      Hashes.emplace_back((I.getRawData())[J]);
+    auto RawVals = ArrayRef<uint64_t>(I.getRawData(), I.getNumWords());
+    Hashes.append(RawVals.begin(), RawVals.end());
     return stable_hash_combine(Hashes);
   }
 
   stable_hash hashAPFloat(const APFloat &F) {
-    SmallVector<stable_hash> Hashes;
-    const fltSemantics &S = F.getSemantics();
-    Hashes.emplace_back(APFloat::semanticsPrecision(S));
-    Hashes.emplace_back(APFloat::semanticsMaxExponent(S));
-    Hashes.emplace_back(APFloat::semanticsMinExponent(S));
-    Hashes.emplace_back(APFloat::semanticsSizeInBits(S));
-    Hashes.emplace_back(hashAPInt(F.bitcastToAPInt()));
-    return stable_hash_combine(Hashes);
+    return hashAPInt(F.bitcastToAPInt());
   }
 
   stable_hash hashGlobalValue(const GlobalValue *GV) {
@@ -106,8 +99,7 @@ class StructuralHashImpl {
       return stable_hash_combine(Hashes);
     }
 
-    auto *G = dyn_cast<GlobalValue>(C);
-    if (G) {
+    if (auto *G = dyn_cast<GlobalValue>(C)) {
       Hashes.emplace_back(hashGlobalValue(G));
       return stable_hash_combine(Hashes);
     }
@@ -135,8 +127,6 @@ class StructuralHashImpl {
     }
     case Value::ConstantArrayVal: {
       const ConstantArray *A = cast<ConstantArray>(C);
-      uint64_t NumElements = cast<ArrayType>(Ty)->getNumElements();
-      Hashes.emplace_back(NumElements);
       for (auto &Op : A->operands()) {
         auto H = hashConstant(cast<Constant>(Op));
         Hashes.emplace_back(H);
@@ -145,8 +135,6 @@ class StructuralHashImpl {
     }
     case Value::ConstantStructVal: {
       const ConstantStruct *S = cast<ConstantStruct>(C);
-      unsigned NumElements = cast<StructType>(Ty)->getNumElements();
-      Hashes.emplace_back(NumElements);
       for (auto &Op : S->operands()) {
         auto H = hashConstant(cast<Constant>(Op));
         Hashes.emplace_back(H);
@@ -155,8 +143,6 @@ class StructuralHashImpl {
     }
     case Value::ConstantVectorVal: {
       const ConstantVector *V = cast<ConstantVector>(C);
-      unsigned NumElements = cast<FixedVectorType>(Ty)->getNumElements();
-      Hashes.emplace_back(NumElements);
       for (auto &Op : V->operands()) {
         auto H = hashConstant(cast<Constant>(Op));
         Hashes.emplace_back(H);
@@ -165,8 +151,6 @@ class StructuralHashImpl {
     }
     case Value::ConstantExprVal: {
       const ConstantExpr *E = cast<ConstantExpr>(C);
-      unsigned NumOperands = E->getNumOperands();
-      Hashes.emplace_back(NumOperands);
       for (auto &Op : E->operands()) {
         auto H = hashConstant(cast<Constant>(Op));
         Hashes.emplace_back(H);
@@ -185,10 +169,10 @@ class StructuralHashImpl {
       Hashes.emplace_back(H);
       return stable_hash_combine(Hashes);
     }
-    default: // Unknown constant, abort.
-      llvm_unreachable("Constant ValueID not recognized.");
+    default:
+      // Skip other types of constants for simplicity.
+      return stable_hash_combine(Hashes);
     }
-    return Hash;
   }
 
   stable_hash hashValue(Value *V) {
@@ -203,8 +187,8 @@ class StructuralHashImpl {
       Hashes.emplace_back(Arg->getArgNo());
 
     // Get an index (an insertion order) for the non-constant value.
-    auto I = ValueToId.insert({V, ValueToId.size()});
-    Hashes.emplace_back(I.first->second);
+    auto [It, WasInserted] = ValueToId.try_emplace(V, ValueToId.size());
+    Hashes.emplace_back(It->second);
 
     return stable_hash_combine(Hashes);
   }
@@ -233,14 +217,14 @@ class StructuralHashImpl {
     unsigned InstIdx = 0;
     if (IndexInstruction) {
       InstIdx = IndexInstruction->size();
-      IndexInstruction->insert({InstIdx, const_cast<Instruction *>(&Inst)});
+      IndexInstruction->try_emplace(InstIdx, const_cast<Instruction *>(&Inst));
     }
 
     for (const auto [OpndIdx, Op] : enumerate(Inst.operands())) {
       auto OpndHash = hashOperand(Op);
       if (IgnoreOp && IgnoreOp(&Inst, OpndIdx)) {
         assert(IndexOperandHashMap);
-        IndexOperandHashMap->insert({{InstIdx, OpndIdx}, OpndHash});
+        IndexOperandHashMap->try_emplace({InstIdx, OpndIdx}, OpndHash);
       } else
         Hashes.emplace_back(OpndHash);
     }

llvm/unittests/IR/StructuralHashTest.cpp

@@ -10,6 +10,7 @@
 #include "llvm/AsmParser/Parser.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/SourceMgr.h"
+#include "gmock/gmock-matchers.h"
 #include "gtest/gtest.h"
 
 #include <memory>
@@ -18,6 +19,11 @@ using namespace llvm;
 
 namespace {
 
+using testing::Contains;
+using testing::Key;
+using testing::Pair;
+using testing::SizeIs;
+
 std::unique_ptr<Module> parseIR(LLVMContext &Context, const char *IR) {
   SMDiagnostic Err;
   std::unique_ptr<Module> M = parseAssemblyString(IR, Err, Context);
@@ -271,16 +277,16 @@ TEST(StructuralHashTest, Differences) {
   EXPECT_EQ(FuncHashInfo1.FunctionHash, FuncHashInfo2.FunctionHash);
 
   // There are total 3 instructions.
-  EXPECT_EQ(FuncHashInfo1.IndexInstruction->size(), 3u);
-  EXPECT_EQ(FuncHashInfo2.IndexInstruction->size(), 3u);
+  EXPECT_THAT(*FuncHashInfo1.IndexInstruction, SizeIs(3));
+  EXPECT_THAT(*FuncHashInfo2.IndexInstruction, SizeIs(3));
 
   // The only 1 operand (the call target) has been ignored.
-  EXPECT_EQ(FuncHashInfo1.IndexOperandHashMap->size(), 1u);
-  EXPECT_EQ(FuncHashInfo2.IndexOperandHashMap->size(), 1u);
+  EXPECT_THAT(*FuncHashInfo1.IndexOperandHashMap, SizeIs(1u));
+  EXPECT_THAT(*FuncHashInfo2.IndexOperandHashMap, SizeIs(1u));
 
   // The index pair of instruction and operand (1, 1) is a key in the map.
-  ASSERT_TRUE(FuncHashInfo1.IndexOperandHashMap->count({1, 1}));
-  ASSERT_TRUE(FuncHashInfo2.IndexOperandHashMap->count({1, 1}));
+  ASSERT_THAT(*FuncHashInfo1.IndexOperandHashMap, Contains(Key(Pair(1, 1))));
+  ASSERT_THAT(*FuncHashInfo2.IndexOperandHashMap, Contains(Key(Pair(1, 1))));
 
   // The indexed instruciton must be the call instruction as shown in the
   // IgnoreOp above.