Bug fix on stable hash calculation for machine operands RegisterMask and RegisterLiveOut

[yozhu]

Summary

MachineOperand::getRegMask() returns a pointer to register mask. We should hash the raw content of register mask instead of its pointer.

[kyulee]

llvm/lib/CodeGen/MachineStableHash.cpp

122–135

Strictly speaking, the mask pointer can be invalid or the mask size can be longer than int value.
Should we iterate it similar to https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/MachineOperand.cpp#L318?

[smeenai]

Is it possible to add a test case to prevent this from regressing in the future? (It;s fine if the answer is "no"; I'm not familiar enough with this part of the code to say.)

[yozhu]

Address comment on needing to consider regmask size; it is not necessarily a uint32_t.

[yozhu]

In D133637#3782281, @smeenai wrote:

Is it possible to add a test case to prevent this from regressing in the future? (It;s fine if the answer is "no"; I'm not familiar enough with this part of the code to say.)

I'm not sure if it is feasible to add a small unit test for this. Stable hash calculated on machine operand doesn't exist in compiler output or produced OBJ or binary, and also it is hard to tell whether a hash value is calculated from hashing pointers or raw data. The problem was manifested as non-determinism in binary. If we test determinism, then the input need to be relatively large (larger than usual unit test) so any inherent randomness could be exposed relatively reliably. But determinism test will verify the whole toolset instead of only testing machine IR stable hash calculation.

[kyulee]

LGTM

[MatzeB]

llvm/include/llvm/CodeGen/MachineOperand.h

644–646

I am not sure this is a good API if it behaves differently depending on whether the instruction is added to a machine function / block yet. This can be very misleading for people using this function, so I think it may have been better not to add a publicly visible function here and rather deal with it within the hash function directly...

llvm/lib/CodeGen/MachineStableHash.cpp

123–135

So this means the hash value changes depending on whether the instruction is attached to a MachineFunction yet?. I find a hash value changing like that dangerous / unexpected to users. Did you try whether you can assert/abort instead if the instruction is not attached to a function?

[efriedma]

llvm/lib/CodeGen/MachineOperand.cpp

394

While you're here, can you also fix llvm::hash_value?

[MatzeB]

An alternative approach if we cannot guarantee the availabily of a MachineFunction reference, may be to change machine operands to store ArrayRef<Register> RegMask instead of uint32_t *RegMask. It shouldn't affect memory usage because MachineOperand size is determined by the biggest union member anyway which is already 2 pointers...

[yozhu]

llvm/lib/CodeGen/MachineOperand.cpp

394

Yes, will do. Thanks for pointing this out!

[yozhu]

Discussed with @MatzeB and @kyulee offline, and we will put up a new diff to remove the new API (so to get rid of confusion) and to duplicate the code in the few places where we need to hash MO. For the concern that MO will have different hash values between when it has an associated MF and when it not, we will add an assert (suggesting that in most cases, if not all, we shouldn't hash MO if it doesn't belong to a MF) but will leave the fallback code as in the current change (for Release build, in case it is a valid scenario somewhere but not covered by the current set of tests).

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

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;
 
-    if (const MachineFunction *MF = getMFIfAvailable(*this)) {
-      // Calculate the size of the RegMask
-      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
-      unsigned RegMaskSize = (TRI->getNumRegs() + 31) / 32;
-
+    const unsigned RegMaskSize = getRegMaskSize();
+    if (RegMaskSize != 0) {
       // 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:
hash_code llvm::hash_value(const MachineOperand &MO) {
   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_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

case MachineOperand::MO_JumpTableIndex:
     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:
-    return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getRegMask());
+  case MachineOperand::MO_RegisterLiveOut: {
+    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()));
+    }
+
+    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),
         [](int S) -> llvm::stable_hash { return llvm::stable_hash(S); });