[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

62

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

62

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

62

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

62

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

65

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

65

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).
-  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;
-  };
-
-  SmallVector<unsigned, 16> MIOperands = {MI.getOpcode(), MI.getFlags()};
-  llvm::transform(MI.uses(), std::back_inserter(MIOperands), GetHashableMO);
+  // Here we walk over all of the flags and operands (non vreg-defs) of the MI
+  // to collect artifacts for hashing. Where we used to walk over all of the
+  // MachoneOperand uses of an MI we now instead use MachineInstrExpressionTrait
+  // since that covers more types of operands and is less likely to result in
+  // collisions. MachineInstrExpressionTrait is what is used to produce hashes
+  // by MachineCSE. So in effect we are hashing on the MI flags, the MI
+  // memoprands, and the hash of all of the other non-vreg def operands of the
+  // MI (the latter through MachineInstrExpressionTrait).
+  SmallVector<unsigned, 16> MIOperands = {MI.getFlags()};
+  MIOperands.push_back(MachineInstrExpressionTrait::getHashValue(&MI));
 
   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());
   }

llvm/test/CodeGen/MIR/AArch64/mirnamer.mir

bb.0:
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = LDRWui
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = MOVi32imm 1
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__2:gpr32 = LDRWui
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = MOVi32imm 2
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__3:gpr32 = LDRWui
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = MOVi32imm 3
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = nsw ADDWrr
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__4:gpr32 = LDRWui
-    ;CHECK-NEXT: %bb0_{{[0-9]+}}__2:gpr32 = nsw ADDWrr
+    ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = nsw ADDWrr
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = MOVi32imm 4
-    ;CHECK-NEXT: %bb0_{{[0-9]+}}__3:gpr32 = nsw ADDWrr
+    ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = nsw ADDWrr
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__5:gpr32 = LDRWui
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = MOVi32imm 5
 
     %0:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)
     %1:gpr32 = MOVi32imm 1
     %2:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)
     %3:gpr32 = MOVi32imm 2
     %4:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)

 body:             |
   bb.0:
     liveins: $x0, $x1, $d0, $d1
 
     ;CHECK-LABEL: bb.0:
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = LDRWui %stack.0, 0
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = COPY %bb0_{{[0-9]+}}__1
     ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = COPY %bb0_{{[0-9]+}}__1
-    ;CHECK-NEXT: %bb0_{{[0-9]+}}__2:gpr32 = COPY %bb0_{{[0-9]+}}__1
-    ;CHECK-NEXT: $w0 = COPY %bb0_{{[0-9]+}}__2
+    ;CHECK-NEXT: %bb0_{{[0-9]+}}__1:gpr32 = COPY %bb0_{{[0-9]+}}__1
+    ;CHECK-NEXT: $w0 = COPY %bb0_{{[0-9]+}}__1
 
     %0:gpr32 = LDRWui %stack.0, 0 :: (dereferenceable load 8)
     %1:gpr32 = COPY %0
     %2:gpr32 = COPY %1
     %3:gpr32 = COPY %2
     $w0 = COPY %3
     RET_ReallyLR implicit $w0
 ...
 

llvm/test/CodeGen/MIR/AMDGPU/mir-canon-multi.mir

bb.0:
     ; CHECK: %bb0_{{[0-9]+}}__1:sreg_32_xm0 = S_MOV_B32 61440
     ; CHECK: %bb0_{{[0-9]+}}__1:sreg_32_xm0 = S_MOV_B32 0
     ; CHECK: %bb0_{{[0-9]+}}__1:vgpr_32 = COPY $vgpr0
     ; CHECK: %bb0_{{[0-9]+}}__1:sgpr_64 = COPY $sgpr0_sgpr1
     ; CHECK: %bb0_{{[0-9]+}}__1:sreg_64_xexec = S_LOAD_DWORDX2_IMM %bb0_{{[0-9]+}}__1, 9, 0, 0
     ; CHECK: %bb0_{{[0-9]+}}__1:sreg_64_xexec = S_LOAD_DWORDX2_IMM %bb0_{{[0-9]+}}__1, 11, 0, 0
     ; CHECK: %bb0_{{[0-9]+}}__1:vgpr_32 = COPY %bb0_{{[0-9]+}}__1
     ; CHECK: %bb0_{{[0-9]+}}__1:vgpr_32 = COPY %bb0_{{[0-9]+}}__1
-    ; CHECK: %bb0_{{[0-9]+}}__2:vgpr_32 = COPY %bb0_{{[0-9]+}}__1
+    ; CHECK: %bb0_{{[0-9]+}}__1:vgpr_32 = COPY %bb0_{{[0-9]+}}__1
     ; CHECK: %bb0_{{[0-9]+}}__1:vreg_64 = REG_SEQUENCE %bb0_{{[0-9]+}}__1, %subreg.sub0, %bb0_{{[0-9]+}}__1, %subreg.sub1
-    ; CHECK: %bb0_{{[0-9]+}}__1:sgpr_128 = REG_SEQUENCE %bb0_{{[0-9]+}}__1, %subreg.sub0, %bb0_{{[0-9]+}}__1, %subreg.sub1, %bb0_{{[0-9]+}}__1, %subreg.sub2, %bb0_{{[0-9]+}}__2, %subreg.sub3
+    ; CHECK: %bb0_{{[0-9]+}}__1:sgpr_128 = REG_SEQUENCE %bb0_{{[0-9]+}}__1, %subreg.sub0, %bb0_{{[0-9]+}}__1, %subreg.sub1, %bb0_{{[0-9]+}}__1, %subreg.sub2, %bb0_{{[0-9]+}}__1, %subreg.sub3
     ; CHECK: BUFFER_STORE_DWORD_ADDR64 %bb0_{{[0-9]+}}__1, %bb0_{{[0-9]+}}__1, %bb0_{{[0-9]+}}__1, 0, 0, 0, 0, 0, 0, 0, implicit $exec
     ; CHECK: S_ENDPGM 0
     %10:sreg_32_xm0 = S_MOV_B32 61440
     %11:sreg_32_xm0 = S_MOV_B32 0
     %3:vgpr_32 = COPY $vgpr0
 
     %vreg123_0:vgpr_32 = COPY %3
     %0:sgpr_64 = COPY $sgpr0_sgpr1