Address feedback in https://reviews.llvm.org/D133637

[yozhu]

Summary

https://reviews.llvm.org/D133637 fixes the problem where we should hash raw content of
register mask instead of the pointer to it.

Fix the same issue in llvm::hash_value().

Remove the added API MachineOperand::getRegMaskSize() to avoid potential confusion.

Add an assert to emphasize that we probably should hash a machine operand iff it has
associated machine function, but keep the fallback logic in the original change.

[yozhu]

Make clang-format happy.

[ellis]

llvm/lib/CodeGen/MachineOperand.cpp

396

I find it a bit strange that we are failing an assert but still returning something. Is this better suited for llvm_unreachable()?

[MatzeB]

LGTM, thanks

llvm/lib/CodeGen/MachineOperand.cpp

329

387–388

llvm/lib/CodeGen/MachineStableHash.cpp

127

[yozhu]

llvm/lib/CodeGen/MachineOperand.cpp

396

The assert() is mainly to get people's attention if a machine operand is hashed when it is not associated with any machine function (since later when it gets associated with a machine function, the hash value, if calculated again, would be different). But we are not sure if the above scenario is absolutely invalid or no one is actually using this, so we made the code still return something (at least the Release build will work if the scenario does exist).

[yozhu]

Use static member function MachineOperand::getRegMaskSize() instead of
duplicating the logic in code.

llvm/include/llvm/CodeGen/MachineOperand.h

public:
 
   /// getRegMask - Returns a bit mask of registers preserved by this RegMask
   /// operand.
   const uint32_t *getRegMask() const {
     assert(isRegMask() && "Wrong MachineOperand accessor");
     return Contents.RegMask;
   }
 
-  /// Return the size of regmask array if we are able to figure it out from
-  /// this operand. Return zero otherwise.
-  unsigned getRegMaskSize() const;
-
   /// Returns number of elements needed for a regmask array.
   static unsigned getRegMaskSize(unsigned NumRegs) {
     return (NumRegs + 31) / 32;
   }
 
   /// getRegLiveOut - Returns a bit mask of live-out registers.
   const uint32_t *getRegLiveOut() const {
     assert(isRegLiveOut() && "Wrong MachineOperand accessor");

llvm/lib/CodeGen/MachineOperand.cpp

 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/Loads.h"
 #include "llvm/CodeGen/MIRFormatter.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/CodeGen/StableHashing.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/ModuleSlotTracker.h"
 #include "llvm/MC/MCDwarf.h"

 
 static const MachineFunction *getMFIfAvailable(const MachineOperand &MO) {
   if (const MachineInstr *MI = MO.getParent())
     if (const MachineBasicBlock *MBB = MI->getParent())
       if (const MachineFunction *MF = MBB->getParent())
         return MF;
   return nullptr;
 }
+
 static MachineFunction *getMFIfAvailable(MachineOperand &MO) {
   return const_cast<MachineFunction *>(
       getMFIfAvailable(const_cast<const MachineOperand &>(MO)));
 }
 
 void MachineOperand::setReg(Register Reg) {
   if (getReg() == Reg)
     return; // No change.
if (!WasReg)
     TiedTo = 0;
 
   // If this operand is embedded in a function, add the operand to the
   // register's use/def list.
   if (RegInfo)
     RegInfo->addRegOperandToUseList(this);
 }
 
-/// getRegMaskSize - Return the size of regmask array if we are able to figure
-/// it out from this operand. Return zero otherwise.
-unsigned MachineOperand::getRegMaskSize() const {
-  if (const MachineFunction *MF = getMFIfAvailable(*this)) {
-    const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
-    unsigned RegMaskSize = (TRI->getNumRegs() + 31) / 32;
-    return RegMaskSize;
-  }
-  return 0;
-}
-
 /// isIdenticalTo - Return true if this operand is identical to the specified
 /// operand. Note that this should stay in sync with the hash_value overload
 /// below.
 bool MachineOperand::isIdenticalTo(const MachineOperand &Other) const {
   if (getType() != Other.getType() ||
       getTargetFlags() != Other.getTargetFlags())
     return false;
 
bool MachineOperand::isIdenticalTo(const MachineOperand &Other) const {
   case MachineOperand::MO_RegisterMask:
   case MachineOperand::MO_RegisterLiveOut: {
     // Shallow compare of the two RegMasks
     const uint32_t *RegMask = getRegMask();
     const uint32_t *OtherRegMask = Other.getRegMask();
     if (RegMask == OtherRegMask)
       return true;
 
-    const unsigned RegMaskSize = getRegMaskSize();
-    if (RegMaskSize != 0) {
+    if (const MachineFunction *MF = getMFIfAvailable(*this)) {
+      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+      unsigned RegMaskSize = MachineOperand::getRegMaskSize(TRI->getNumRegs());
       // Deep compare of the two RegMasks
       return std::equal(RegMask, RegMask + RegMaskSize, OtherRegMask);
     }
     // We don't know the size of the RegMask, so we can't deep compare the two
     // reg masks.
     return false;
   }
   case MachineOperand::MO_MCSymbol:
return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getOffset(),
                         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:
-    return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getRegMask());
+  case MachineOperand::MO_RegisterLiveOut: {
+    if (const MachineFunction *MF = getMFIfAvailable(MO)) {
+      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+      unsigned RegMaskSize = MachineOperand::getRegMaskSize(TRI->getNumRegs());
+      const uint32_t *RegMask = MO.getRegMask();
+      std::vector<stable_hash> RegMaskHashes(RegMask, RegMask + RegMaskSize);
+      return hash_combine(MO.getType(), MO.getTargetFlags(),
+                          stable_hash_combine_array(RegMaskHashes.data(),
+                                                    RegMaskHashes.size()));
+    }
+
+    assert(0 && "MachineOperand not associated with any MachineFunction");
+    return hash_combine(MO.getType(), MO.getTargetFlags());
+  }
   case MachineOperand::MO_Metadata:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getMetadata());
   case MachineOperand::MO_MCSymbol:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getMCSymbol());
   case MachineOperand::MO_CFIIndex:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getCFIIndex());
   case MachineOperand::MO_IntrinsicID:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getIntrinsicID());

llvm/lib/CodeGen/MachineStableHash.cpp

return stable_hash_combine(MO.getType(), MO.getTargetFlags(),
                                MO.getIndex());
 
   case MachineOperand::MO_ExternalSymbol:
     return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getOffset(),
                         stable_hash_combine_string(MO.getSymbolName()));
 
   case MachineOperand::MO_RegisterMask:
   case MachineOperand::MO_RegisterLiveOut: {
+    if (const MachineInstr *MI = MO.getParent()) {
+      if (const MachineBasicBlock *MBB = MI->getParent()) {
+        if (const MachineFunction *MF = MBB->getParent()) {
+          const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+          unsigned RegMaskSize =
+              MachineOperand::getRegMaskSize(TRI->getNumRegs());
           const uint32_t *RegMask = MO.getRegMask();
-    const unsigned RegMaskSize = MO.getRegMaskSize();
-
-    if (RegMaskSize != 0) {
           std::vector<llvm::stable_hash> RegMaskHashes(RegMask,
                                                        RegMask + RegMaskSize);
           return hash_combine(MO.getType(), MO.getTargetFlags(),
                               stable_hash_combine_array(RegMaskHashes.data(),
                                                         RegMaskHashes.size()));
         }
+      }
+    }
 
+    assert(0 && "MachineOperand not associated with any MachineFunction");
     return hash_combine(MO.getType(), MO.getTargetFlags());
   }
 
   case MachineOperand::MO_ShuffleMask: {
     std::vector<llvm::stable_hash> ShuffleMaskHashes;
 
     llvm::transform(
         MO.getShuffleMask(), std::back_inserter(ShuffleMaskHashes),