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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi created this revision.
plotfi added reviewers: bogner, aditya_nandakumar, compnerd.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

I noticed that there exists a handy hash_value function that covers all the basis for the hashing of various types of machine operand types and I think this cleans up a bunch of the hashing code for a given MachineInstr.

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.


Repository:
  rG LLVM Github Monorepo

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,17 @@
   std::string S;
   raw_string_ostream OS(S);
 
-  // Gets a hashable artifact from a given MachineOperand (ie an unsigned).
-  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
-    // 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;
+  // 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 = [](const MachineOperand &MO) {
+    return llvm::hash_value(MO);
   };
 
   SmallVector<unsigned, 16> MIOperands = {MI.getOpcode(), MI.getFlags()};


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