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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi updated this revision to Diff 233666.
plotfi added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71396

Files:
  llvm/lib/CodeGen/MIRVRegNamerUtils.cpp


Index: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
===================================================================
--- llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
+++ llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
@@ -50,23 +50,58 @@
   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()))
-      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
+    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();
+      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.
-    return 0;
+    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()};


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71396.233666.patch
Type: text/x-patch
Size: 3713 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191212/2fbc7bd9/attachment.bin>