[llvm][NFCi][lMIRVRegNamerUtils] Leverage hash_value for hashing a MachineInstr.

[plotfi]

Summary

I noticed that hash_value does a lot of what we want to do with the hashing of operands for the MIRVRegNamer and that it also covers all the basis for the hashing of various types of machine operand types that was not being handled before (llvm::hash_value(MachineOperand) appears to cover everything).

Still trying to think of a good way to test this other than adding MachineInstrs to a test that have some of the other types of MOs. I think since for instance in the case of an immediate MO and other types too, instead of returning just the immediate we are returning the hash of the immediate along with the target flags for the operand and the type (MO_Immediate), that we are going to get different hashes for the totality of the MachineInstr. I still think this will achieve the objectives here though.

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

117

While I think this is cleaner, and I was aware of this, and one of the reasons I didn't use it initially is that for things such as PHI instructions, it just hashes the BB with just the pointer value (casted as integer) which is not guaranteed to be stable across different runs (but perfectly usable within the same run) which will affect the diffs.
Perhaps a compromise which we can do is use the hash defined in the machine operand but opt out of hashing if we're dealing with MBB type.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

117

Yeah agreed. I can revert back to https://reviews.llvm.org/D71396?id=233516 and try using hash_value directly on everything except for the specific things you've mentioned.

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

117

Sounds good.

[plotfi]

Being more explicit about which MachineOperand types can be hashed and which ones can be skipped due to unstable hashing on pointers as @aditya_nandakumar mentioned. Also brought back hashing on a vreg's defining MI operand because that makes sense.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

117

I think I've covered all the basis for things that might be unstable across runs, or might be based on some symbol name that I just am not ready to figure out the hash on. I think for the most part the behavior here should be about the same as before the the exception of some of the Index types and the C/FPImmediates that are backed APFloat/APInt.

[aditya_nandakumar]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

76

This feels redundant - I wonder if instead of hashing the opcode (which we already do below), we could hash something unique about the definition (Reg Class/Reg Bank/LLT). That way

x(s32), y(s32) = FOO <Ops>
x(someregclass), y(s64) = FOO <Ops>

the above instructions will hash differently.

[plotfi]

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

76

I think we could hash the regclass/bank along with the opcode. I thought the point of hashing on the opcode directly coming off the def instead of the vreg itself was the main reason the linear hashing technique doesn’t have the ripple effects from previous renaming techniques.

I wonder why the register class isn’t being used for hashing at https://github.com/llvm/llvm-project/blob/6abd01e4624a2c9f8f76e11cc5d57cc7551b5d2a/llvm/lib/CodeGen/MachineOperand.cpp#L347

[plotfi]

adding hash against RC/RB

[plotfi]

I'm going to create a new diff that is purely NFC, but moves this code to use the switch-statement. I will add multiple diffs them for specific things like CImm/FPImm. I'll eventually abandon this diff.

[plotfi]

Abandoning. Will post some piecemeal patches that handle the various MO types separately.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp

VRegRenamer::getVRegRenameMap(const std::vector<NamedVReg> &VRegs) {
   }
   return VRM;
 }
 
 std::string VRegRenamer::getInstructionOpcodeHash(MachineInstr &MI) {
   std::string S;
   raw_string_ostream OS(S);
 
-  // Gets a hashable artifact from a given MachineOperand (ie an unsigned).
+  // Gets a hashable artifact from a given MachineOperand (ie an unsigned). This
+  // used to be the vreg/physreg number or the immediate value, but various
+  // MachineOperand types used to be limited to just immediates, vregs/physregs,
+  // and Target Indices. This still misses a lot of artifacts like the MO type,
+  // the target flags for the MO, as well as a lot of the other operand types
+  // where we used to just return 0. Instead, it makes sense to leverage the
+  // existing MachineOperand hashing function and just has the combine range of
+  // those hashes. We should have fewer collisions then and maybe we can even
+  // eventually drop the collision handling code.
   auto GetHashableMO = [this](const MachineOperand &MO) -> unsigned {
-    if (MO.isImm())
-      return MO.getImm();
-    if (MO.isTargetIndex())
-      return MO.getOffset() | (MO.getTargetFlags() << 16);
-    if (MO.isReg() && Register::isVirtualRegister(MO.getReg()))
+    switch (MO.getType()) {
+    case MachineOperand::MO_CImmediate:
+      return hash_combine(MO.getType(), MO.getTargetFlags(),
+                          MO.getCImm()->getZExtValue());
+    case MachineOperand::MO_FPImmediate:
+      return hash_combine(
+          MO.getType(), MO.getTargetFlags(),
+          MO.getFPImm()->getValueAPF().bitcastToAPInt().getZExtValue());
+    case MachineOperand::MO_Register:
+      // In the case of a vreg, it's better to hash on the opcode since that
+      // has more semantic meaning that whatever the vreg number is. For
+      // physregs we do want to hash on the register itself.
+      if (Register::isVirtualRegister(MO.getReg()))
         return MRI.getVRegDef(MO.getReg())->getOpcode();
-    if (MO.isReg())
-      return MO.getReg();
-    // TODO:
-    // We could explicitly handle all the types of the MachineOperand,
-    // here but we can just return a common number until we find a
+      LLVM_FALLTHROUGH;
+    case MachineOperand::MO_Immediate:
+    case MachineOperand::MO_FrameIndex:
+    case MachineOperand::MO_ConstantPoolIndex:
+    case MachineOperand::MO_TargetIndex:
+    case MachineOperand::MO_JumpTableIndex:
+    case MachineOperand::MO_CFIIndex:
+    case MachineOperand::MO_IntrinsicID:
+    case MachineOperand::MO_Predicate:
+      return llvm::hash_value(MO);
+    // We explicitly handle all types of MachineOperands that will result in a
+    // stable hash. In the cases below we havn't found a way to that. Instead
+    // we return a common number until we find a
     // compelling test case where this is bad. The only side effect here
     // is contributing to a hash collision but there's enough information
     // (Opcodes,other registers etc) that this will likely not be a problem.
+    case MachineOperand::MO_MachineBasicBlock:
+    case MachineOperand::MO_ExternalSymbol:
+    case MachineOperand::MO_GlobalAddress:
+    case MachineOperand::MO_BlockAddress:
+    case MachineOperand::MO_RegisterMask:
+    case MachineOperand::MO_RegisterLiveOut:
+    case MachineOperand::MO_Metadata:
+    case MachineOperand::MO_MCSymbol:
+    case MachineOperand::MO_ShuffleMask:
       return 0;
+    }
+    llvm_unreachable("Unexpected MachineOperandType.");
   };
 
   SmallVector<unsigned, 16> MIOperands = {MI.getOpcode(), MI.getFlags()};
   llvm::transform(MI.uses(), std::back_inserter(MIOperands), GetHashableMO);
 
   for (const auto *Op : MI.memoperands()) {
     MIOperands.push_back((unsigned)Op->getSize());
     MIOperands.push_back((unsigned)Op->getFlags());
     MIOperands.push_back((unsigned)Op->getOffset());
     MIOperands.push_back((unsigned)Op->getOrdering());
     MIOperands.push_back((unsigned)Op->getAddrSpace());
     MIOperands.push_back((unsigned)Op->getSyncScopeID());
     MIOperands.push_back((unsigned)Op->getBaseAlignment());
     MIOperands.push_back((unsigned)Op->getFailureOrdering());
   }
 
   auto HashMI = hash_combine_range(MIOperands.begin(), MIOperands.end());
   return std::to_string(HashMI).substr(0, 5);
 }
 
 unsigned VRegRenamer::createVirtualRegister(unsigned VReg) {