[CodeGen] Fix hashing for MO_ExternalSymbol MachineOperands.

[efriedma]

Summary

We were hashing the string pointer, not the string, so two instructions could be identical (isIdenticalTo), but have different hash codes.

This showed up as a very rare, non-deterministic assertion failure rehashing a DenseMap constructed by MachineOutliner. So there's no "real" testcase, just a unittest which checks that the hash function behaves correctly.

I'm a little scared fixing this is going to cause a regression in outlining or MachineCSE, but hopefully we won't run into any issues.

[kristina]

Seems good. Would it make sense to also add a test explicitly using llvm::hash_combine, as well as a negative test where hash code comparison fails for a literal and the same literal in a StringRef every time (if it's possible to do reliably)? Just a random suggestion, probably overly paranoid.

[efriedma]

Would it make sense to also add a test explicitly using llvm::hash_combine

Using llvm::hash_combine on what, exactly?

as well as a negative test where hash code comparison fails for a literal and the same literal in a StringRef every time

You mean something like ASSERT_NE(SymName1.data(), SymName2.data());?

[kristina]

In D61975#1519367, @efriedma wrote:

Would it make sense to also add a test explicitly using llvm::hash_combine

Using llvm::hash_combine on what, exactly?

as well as a negative test where hash code comparison fails for a literal and the same literal in a StringRef every time

You mean something like ASSERT_NE(SymName1.data(), SymName2.data());?

I just realized that this was for specialization of hash_value for MO, I'm blind, disregard the first remark, sorry.

Regarding second, I meant a case of performing hash_combine manually, and then using ASSERT_NE, with first case being a string literal, and second being the same literal but passed in as StringRef. Assuming it's possible to have a string that would never yield identical hash result to hashing the pointer to a literal. I'm not sure if that makes sense as a test or not, thinking about it, probably doesn't aside from "just for the sake of it/out of paranoia". Was just a suggestion, if it doesn't make sense to test it, ignore the remark.

[efriedma]

I looked more carefully just as I was about to commit this, and discovered the hashing for MO_RegisterMask is also wrong. I think I'll fix that separately, though.

llvm/trunk/lib/CodeGen/MachineOperand.cpp

hash_code llvm::hash_value(const MachineOperand &MO) {
   case MachineOperand::MO_ConstantPoolIndex:
   case MachineOperand::MO_TargetIndex:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getIndex(),
                         MO.getOffset());
   case MachineOperand::MO_JumpTableIndex:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getIndex());
   case MachineOperand::MO_ExternalSymbol:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getOffset(),
-                        MO.getSymbolName());
+                        StringRef(MO.getSymbolName()));
   case MachineOperand::MO_GlobalAddress:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getGlobal(),
                         MO.getOffset());
   case MachineOperand::MO_BlockAddress:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getBlockAddress(),
                         MO.getOffset());
   case MachineOperand::MO_RegisterMask:
   case MachineOperand::MO_RegisterLiveOut:

llvm/trunk/unittests/CodeGen/MachineOperandTest.cpp

TEST(MachineOperandTest, PrintPredicate) {
 
   std::string str;
   // Print a MachineOperand containing a int predicate ICMP_EQ.
   raw_string_ostream OS(str);
   MO.print(OS, /*TRI=*/nullptr, /*IntrinsicInfo=*/nullptr);
   ASSERT_TRUE(OS.str() == "intpred(eq)");
 }
 
+TEST(MachineOperandTest, HashValue) {
+  char SymName1[] = "test";
+  char SymName2[] = "test";
+  MachineOperand MO1 = MachineOperand::CreateES(SymName1);
+  MachineOperand MO2 = MachineOperand::CreateES(SymName2);
+  ASSERT_NE(SymName1, SymName2);
+  ASSERT_EQ(hash_value(MO1), hash_value(MO2));
+  ASSERT_TRUE(MO1.isIdenticalTo(MO2));
+}
+
 } // end namespace