[PATCH] D71396: [llvm][NFCi][lMIRVRegNamerUtils] Leverage hash_value for hashing a MachineInstr.

Thu Dec 12 12:28:41 PST 2019

plotfi marked an inline comment as done.
plotfi added inline comments.

================
Comment at: llvm/lib/CodeGen/MIRVRegNamerUtils.cpp:62
+  SmallVector<unsigned, 16> MIOperands = {MI.getFlags()};
+  MIOperands.push_back(MachineInstrExpressionTrait::getHashValue(&MI));

----------------
aditya_nandakumar wrote:
> plotfi wrote:
> > aditya_nandakumar wrote:
> > > 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.
> > 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. 
> Sounds good.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71396