[CodeGen] Fix hashing for RegMask operands.

[efriedma]

Summary

The code was hashing a pointer to the mask, not the mask itself.

The critical change here is the change to hash_value in MachineOperand.cpp; the other changes are just to pass around the size of the mask.

I'm not sure if this fixes any visible issues in the places hashing is currently used; this was found by inspection while looking into D61975.

[arsenm]

I think the way register masks are used, the pointer value is almost OK. The typical case is a tablgenerated target constant, which will be a unique address. The IPRA case does use non-constants, and I don't think it tries to unique them (although I suppose it could)

llvm/unittests/CodeGen/MachineOperandTest.cpp

416–418

These should use EXPECT

[efriedma]

The typical case is a tablgenerated target constant, which will be a unique address.

Are tablegen'ed constants guaranteed to be unique, or does it depend on the way each target writes its TableGen files?

(although I suppose it could)

Do you think this is worth doing?

[arsenm]

In D70698#1767973, @efriedma wrote:

The typical case is a tablgenerated target constant, which will be a unique address.

Are tablegen'ed constants guaranteed to be unique, or does it depend on the way each target writes its TableGen files?

I don't think these will be value unique. The dynamic RegMask case used for IPRA throws this away

(although I suppose it could)

Do you think this is worth doing?

Might as well fix it, but I doubt it's possible to really break anything with this

llvm/include/llvm/CodeGen/MachineInstrBuilder.h

 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBundle.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
+#include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <cstdint>
 #include <utility>
 
 namespace llvm {
public:
   const MachineInstrBuilder &addBlockAddress(const BlockAddress *BA,
                                              int64_t Offset = 0,
                                              unsigned TargetFlags = 0) const {
     MI->addOperand(*MF, MachineOperand::CreateBA(BA, Offset, TargetFlags));
     return *this;
   }
 
   const MachineInstrBuilder &addRegMask(const uint32_t *Mask) const {
-    MI->addOperand(*MF, MachineOperand::CreateRegMask(Mask));
+    uint32_t NumRegs = MF->getSubtarget().getRegisterInfo()->getNumRegs();
+    uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
+    MI->addOperand(*MF, MachineOperand::CreateRegMask(Mask, Words));
     return *this;
   }
 
   const MachineInstrBuilder &addMemOperand(MachineMemOperand *MMO) const {
     MI->addMemOperand(*MF, MMO);
     return *this;
   }
 

llvm/include/llvm/CodeGen/MachineOperand.h

private:
   MachineInstr *ParentMI;
 
   /// Contents union - This contains the payload for the various operand types.
   union {
     MachineBasicBlock *MBB;  // For MO_MachineBasicBlock.
     const ConstantFP *CFP;   // For MO_FPImmediate.
     const ConstantInt *CI;   // For MO_CImmediate. Integers > 64bit.
     int64_t ImmVal;          // For MO_Immediate.
-    const uint32_t *RegMask; // For MO_RegisterMask and MO_RegisterLiveOut.
     const MDNode *MD;        // For MO_Metadata.
     MCSymbol *Sym;           // For MO_MCSymbol.
     unsigned CFIIndex;       // For MO_CFI.
     Intrinsic::ID IntrinsicID; // For MO_IntrinsicID.
     unsigned Pred;           // For MO_Predicate
     const Constant *ShuffleMask; // For MO_ShuffleMask
 
+    // For MO_RegisterMask and MO_RegisterLiveOut.
+    //
+    // We store the size mostly for convenience; it's invariant for each
+    // subtarget, but some APIs don't have any easy way to look up the current
+    // subtarget. (We could change how those APIs work, but there isn't
+    // really any point; this union is at least two pointers wide for other
+    // reasons.)
+    struct {
+      const uint32_t *Ptr;
+      uint32_t Words;
+    } RegMask;
+
     struct {                  // For MO_Register.
       // Register number is in SmallContents.RegNo.
       MachineOperand *Prev;   // Access list for register. See MRI.
       MachineOperand *Next;
     } Reg;
 
     /// OffsetedInfo - This struct contains the offset and an object identifier.
     /// this represent the object as with an optional offset from it.
public:
   bool clobbersPhysReg(unsigned PhysReg) const {
      return clobbersPhysReg(getRegMask(), PhysReg);
   }
 
   /// 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 Contents.RegMask.Ptr;
+  }
+
+  /// getRegMask - Returns a bit mask of registers preserved by this RegMask
+  /// operand, as an ArrayRef.
+  const ArrayRef<uint32_t> getRegMaskArray() const {
+    return ArrayRef<uint32_t>(Contents.RegMask.Ptr, Contents.RegMask.Words);
   }
 
   /// 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");
-    return Contents.RegMask;
+    return Contents.RegMask.Ptr;
+  }
+
+  /// getRegLiveOut - Returns a bit mask of live-out registers, as an ArrayRef.
+  const ArrayRef<uint32_t> getRegLiveOutArray() const {
+    return ArrayRef<uint32_t>(Contents.RegMask.Ptr, Contents.RegMask.Words);
   }
 
   const MDNode *getMetadata() const {
     assert(isMetadata() && "Wrong MachineOperand accessor");
     return Contents.MD;
   }
 
   //===--------------------------------------------------------------------===//
void setMBB(MachineBasicBlock *MBB) {
     assert(isMBB() && "Wrong MachineOperand mutator");
     Contents.MBB = MBB;
   }
 
   /// Sets value of register mask operand referencing Mask.  The
   /// operand does not take ownership of the memory referenced by Mask, it must
   /// remain valid for the lifetime of the operand. See CreateRegMask().
   /// Any physreg with a 0 bit in the mask is clobbered by the instruction.
-  void setRegMask(const uint32_t *RegMaskPtr) {
+  void setRegMask(const uint32_t *RegMaskPtr, uint32_t RegMaskWords) {
     assert(isRegMask() && "Wrong MachineOperand mutator");
-    Contents.RegMask = RegMaskPtr;
+    Contents.RegMask.Ptr = RegMaskPtr;
+    Contents.RegMask.Words = RegMaskWords;
   }
 
   void setPredicate(unsigned Predicate) {
     assert(isPredicate() && "Wrong MachineOperand mutator");
     Contents.Pred = Predicate;
   }
 
   //===--------------------------------------------------------------------===//
public:
   /// A RegMask operand represents a set of non-clobbered physical registers
   /// on an instruction that clobbers many registers, typically a call.  The
   /// bit mask has a bit set for each physreg that is preserved by this
   /// instruction, as described in the documentation for
   /// TargetRegisterInfo::getCallPreservedMask().
   ///
   /// Any physreg with a 0 bit in the mask is clobbered by the instruction.
   ///
-  static MachineOperand CreateRegMask(const uint32_t *Mask) {
+  static MachineOperand CreateRegMask(const uint32_t *Mask,
+                                      uint32_t MaskWords) {
     assert(Mask && "Missing register mask");
     MachineOperand Op(MachineOperand::MO_RegisterMask);
-    Op.Contents.RegMask = Mask;
+    Op.Contents.RegMask.Ptr = Mask;
+    Op.Contents.RegMask.Words = MaskWords;
     return Op;
   }
-  static MachineOperand CreateRegLiveOut(const uint32_t *Mask) {
+  static MachineOperand CreateRegLiveOut(const uint32_t *Mask,
+                                         uint32_t MaskWords) {
     assert(Mask && "Missing live-out register mask");
     MachineOperand Op(MachineOperand::MO_RegisterLiveOut);
-    Op.Contents.RegMask = Mask;
+    Op.Contents.RegMask.Ptr = Mask;
+    Op.Contents.RegMask.Words = MaskWords;
     return Op;
   }
   static MachineOperand CreateMetadata(const MDNode *Meta) {
     MachineOperand Op(MachineOperand::MO_Metadata);
     Op.Contents.MD = Meta;
     return Op;
   }
 

llvm/lib/CodeGen/MIRParser/MIParser.cpp

while (true) {
     // TODO: Report an error if the same register is used more than once.
     if (Token.isNot(MIToken::comma))
       break;
     lex();
   }
 
   if (expectAndConsume(MIToken::rparen))
     return true;
-  Dest = MachineOperand::CreateRegMask(Mask);
+  uint32_t NumRegs = MF.getSubtarget().getRegisterInfo()->getNumRegs();
+  uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
+  Dest = MachineOperand::CreateRegMask(Mask, Words);
   return false;
 }
 
 bool MIParser::parseLiveoutRegisterMaskOperand(MachineOperand &Dest) {
   assert(Token.is(MIToken::kw_liveout));
   uint32_t *Mask = MF.allocateRegMask();
   lex();
   if (expectAndConsume(MIToken::lparen))
     return true;
   while (true) {
     if (Token.isNot(MIToken::NamedRegister))
       return error("expected a named register");
     unsigned Reg;
     if (parseNamedRegister(Reg))
       return true;
     lex();
     Mask[Reg / 32] |= 1U << (Reg % 32);
     // TODO: Report an error if the same register is used more than once.
     if (Token.isNot(MIToken::comma))
       break;
     lex();
   }
   if (expectAndConsume(MIToken::rparen))
     return true;
-  Dest = MachineOperand::CreateRegLiveOut(Mask);
+  uint32_t NumRegs = MF.getSubtarget().getRegisterInfo()->getNumRegs();
+  uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
+  Dest = MachineOperand::CreateRegLiveOut(Mask, Words);
   return false;
 }
 
 bool MIParser::parseMachineOperand(MachineOperand &Dest,
                                    Optional<unsigned> &TiedDefIdx) {
   switch (Token.kind()) {
   case MIToken::kw_implicit:
   case MIToken::kw_implicit_define:
bool MIParser::parseMachineOperand(MachineOperand &Dest,
   case MIToken::kw_intpred:
     return parsePredicateOperand(Dest);
   case MIToken::kw_shufflemask:
     return parseShuffleMaskOperand(Dest);
   case MIToken::Error:
     return true;
   case MIToken::Identifier:
     if (const auto *RegMask = PFS.Target.getRegMask(Token.stringValue())) {
-      Dest = MachineOperand::CreateRegMask(RegMask);
+      uint32_t NumRegs = MF.getSubtarget().getRegisterInfo()->getNumRegs();
+      uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
+      Dest = MachineOperand::CreateRegMask(RegMask, Words);
       lex();
       break;
     } else if (Token.stringValue() == "CustomRegMask") {
       return parseCustomRegisterMaskOperand(Dest);
     } else
       return parseTypedImmediateOperand(Dest);
   default:
     // FIXME: Parse the MCSymbol machine operand.

llvm/lib/CodeGen/MachineOperand.cpp

case MachineOperand::MO_GlobalAddress:
     return getGlobal() == Other.getGlobal() && getOffset() == Other.getOffset();
   case MachineOperand::MO_ExternalSymbol:
     return strcmp(getSymbolName(), Other.getSymbolName()) == 0 &&
            getOffset() == Other.getOffset();
   case MachineOperand::MO_BlockAddress:
     return getBlockAddress() == Other.getBlockAddress() &&
            getOffset() == Other.getOffset();
   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;
-
-      // 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;
-  }
+    return getRegMaskArray() == Other.getRegMaskArray();
+  case MachineOperand::MO_RegisterLiveOut:
+    return getRegLiveOutArray() == Other.getRegLiveOutArray();
   case MachineOperand::MO_MCSymbol:
     return getMCSymbol() == Other.getMCSymbol();
   case MachineOperand::MO_CFIIndex:
     return getCFIIndex() == Other.getCFIIndex();
   case MachineOperand::MO_Metadata:
     return getMetadata() == Other.getMetadata();
   case MachineOperand::MO_IntrinsicID:
     return getIntrinsicID() == Other.getIntrinsicID();
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:
+    return hash_combine(MO.getType(), MO.getTargetFlags(),
+                        MO.getRegMaskArray());
   case MachineOperand::MO_RegisterLiveOut:
-    return hash_combine(MO.getType(), MO.getTargetFlags(), MO.getRegMask());
+    return hash_combine(MO.getType(), MO.getTargetFlags(),
+                        MO.getRegLiveOutArray());
   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/RegUsageInfoPropagate.cpp

private:
   static void setRegMask(MachineInstr &MI, ArrayRef<uint32_t> RegMask) {
     assert(RegMask.size() ==
            MachineOperand::getRegMaskSize(MI.getParent()->getParent()
                                           ->getRegInfo().getTargetRegisterInfo()
                                           ->getNumRegs())
            && "expected register mask size");
     for (MachineOperand &MO : MI.operands()) {
       if (MO.isRegMask())
-        MO.setRegMask(RegMask.data());
+        MO.setRegMask(RegMask.data(), RegMask.size());
     }
   }
 };
 
 } // end of anonymous namespace
 
 INITIALIZE_PASS_BEGIN(RegUsageInfoPropagation, "reg-usage-propagation",
                       RUIP_NAME, false, false)

llvm/lib/CodeGen/SelectionDAG/FastISel.cpp

bool FastISel::selectPatchpoint(const CallInst *I) {
   for (auto Reg : CLI.OutRegs)
     Ops.push_back(MachineOperand::CreateReg(Reg, /*isDef=*/false));
 
   // Push live variables for the stack map.
   if (!addStackMapLiveVars(Ops, I, NumMetaOpers + NumArgs))
     return false;
 
   // Push the register mask info.
+  uint32_t NumRegs = MF->getSubtarget().getRegisterInfo()->getNumRegs();
+  uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
   Ops.push_back(MachineOperand::CreateRegMask(
-      TRI.getCallPreservedMask(*FuncInfo.MF, CC)));
+      TRI.getCallPreservedMask(*FuncInfo.MF, CC), Words));
 
   // Add scratch registers as implicit def and early clobber.
   const MCPhysReg *ScratchRegs = TLI.getScratchRegisters(CC);
   for (unsigned i = 0; ScratchRegs[i]; ++i)
     Ops.push_back(MachineOperand::CreateReg(
         ScratchRegs[i], /*isDef=*/true, /*isImp=*/true, /*isKill=*/false,
         /*isDead=*/false, /*isUndef=*/false, /*isEarlyClobber=*/true));
 

llvm/lib/CodeGen/StackMapLivenessAnalysis.cpp

bool StackMapLiveness::calculateLiveness(MachineFunction &MF) {
   }
   return HasChanged;
 }
 
 /// Add the current register live set to the instruction.
 void StackMapLiveness::addLiveOutSetToMI(MachineFunction &MF,
                                          MachineInstr &MI) {
   uint32_t *Mask = createRegisterMask(MF);
-  MachineOperand MO = MachineOperand::CreateRegLiveOut(Mask);
+  uint32_t NumRegs = MF.getSubtarget().getRegisterInfo()->getNumRegs();
+  uint32_t Words = MachineOperand::getRegMaskSize(NumRegs);
+  MachineOperand MO = MachineOperand::CreateRegLiveOut(Mask, Words);
   MI.addOperand(MF, MO);
 }
 
 /// Create a register mask and initialize it with the registers from the
 /// register live set.
 uint32_t *StackMapLiveness::createRegisterMask(MachineFunction &MF) const {
   // The mask is owned and cleaned up by the Machine Function.
   uint32_t *Mask = MF.allocateRegMask();
   for (auto Reg : LiveRegs)
     Mask[Reg / 32] |= 1U << (Reg % 32);
 
   // Give the target a chance to adjust the mask.
   TRI->adjustStackMapLiveOutMask(Mask);
 
   return Mask;
 }

llvm/unittests/CodeGen/MachineOperandTest.cpp

TEST(MachineOperandTest, ChangeToTargetIndexTest) {
   ASSERT_TRUE(MO.isTargetIndex());
   ASSERT_TRUE(MO.getIndex() == 74);
   ASSERT_TRUE(MO.getOffset() == 57);
   ASSERT_TRUE(MO.getTargetFlags() == 12);
 }
 
 TEST(MachineOperandTest, PrintRegisterMask) {
   uint32_t Dummy;
-  MachineOperand MO = MachineOperand::CreateRegMask(&Dummy);
+  MachineOperand MO = MachineOperand::CreateRegMask(&Dummy, 1);
 
   // Checking some preconditions on the newly created
   // MachineOperand.
   ASSERT_TRUE(MO.isRegMask());
   ASSERT_TRUE(MO.getRegMask() == &Dummy);
 
   // Print a MachineOperand containing a RegMask. Here we check that without a
   // TRI and IntrinsicInfo we still print a less detailed regmask.
// Print a MachineOperand containing a global address and a negative offset.
     MO.print(OS, /*TRI=*/nullptr, /*IntrinsicInfo=*/nullptr);
     ASSERT_TRUE(OS.str() == "@foo - 12");
   }
 }
 
 TEST(MachineOperandTest, PrintRegisterLiveOut) {
   // Create a MachineOperand with a register live out list and print it.
   uint32_t Mask = 0;
-  MachineOperand MO = MachineOperand::CreateRegLiveOut(&Mask);
+  MachineOperand MO = MachineOperand::CreateRegLiveOut(&Mask, 1);
 
   // Checking some preconditions on the newly created
   // MachineOperand.
   ASSERT_TRUE(MO.isRegLiveOut());
   ASSERT_TRUE(MO.getRegLiveOut() == &Mask);
 
   std::string str;
   // Print a MachineOperand containing a register live out list without a TRI.
TEST(MachineOperandTest, PrintPredicate) {
 
   std::string str;
   // Print a MachineOperand containing a int predicate ICMP_EQ.
   raw_string_ostream OS(str);
   MO.print(OS, /*TRI=*/nullptr, /*IntrinsicInfo=*/nullptr);
   ASSERT_TRUE(OS.str() == "intpred(eq)");
 }
 
-TEST(MachineOperandTest, HashValue) {
+TEST(MachineOperandTest, HashValueSym) {
   char SymName1[] = "test";
   char SymName2[] = "test";
   MachineOperand MO1 = MachineOperand::CreateES(SymName1);
   MachineOperand MO2 = MachineOperand::CreateES(SymName2);
   ASSERT_NE(SymName1, SymName2);
   ASSERT_EQ(hash_value(MO1), hash_value(MO2));
   ASSERT_TRUE(MO1.isIdenticalTo(MO2));
 }
 
+TEST(MachineOperandTest, HashValueRegMask) {
+  uint32_t Mask1 = 1;
+  uint32_t Mask2 = 1;
+  MachineOperand MO1 = MachineOperand::CreateRegMask(&Mask1, 1);
+  MachineOperand MO2 = MachineOperand::CreateRegMask(&Mask2, 1);
+  ASSERT_NE(&Mask1, &Mask2);
+  ASSERT_EQ(hash_value(MO1), hash_value(MO2));
+  ASSERT_TRUE(MO1.isIdenticalTo(MO2));
+}
 } // end namespace