[PATCH] D133637: Bug fix on stable hash calculation for machine operands RegisterMask and RegisterLiveOut

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineOperand.h:644-646
+  /// Return the size of regmask array if we are able to figure it out from
+  /// this operand. Return zero otherwise.
+  unsigned getRegMaskSize() const;
----------------
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...


================
Comment at: llvm/lib/CodeGen/MachineStableHash.cpp:123-135
+    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(),
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133637/new/

https://reviews.llvm.org/D133637