Page MenuHomePhabricator

[llvm][NFCi][lMIRVRegNamerUtils] Leverage hash_value for hashing a MachineInstr.
AbandonedPublic

Authored by plotfi on Dec 11 2019, 11:45 PM.

Details

Summary

I noticed that hash_value does a lot of what we want to do with the hashing of operands for the MIRVRegNamer and that it also covers all the basis for the hashing of various types of machine operand types that was not being handled before (llvm::hash_value(MachineOperand) appears to cover everything).

Still trying to think of a good way to test this other than adding MachineInstrs to a test that have some of the other types of MOs. I think since for instance in the case of an immediate MO and other types too, instead of returning just the immediate we are returning the hash of the immediate along with the target flags for the operand and the type (MO_Immediate), that we are going to get different hashes for the totality of the MachineInstr. I still think this will achieve the objectives here though.

Diff Detail

Event Timeline

plotfi created this revision.Dec 11 2019, 11:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2019, 11:45 PM
plotfi retitled this revision from [llvm][NFCi][lMIRVRegNamerUtils] Leverage llvm::hash_value(MachineOperand) for hashing a MachineInstr. to [llvm][NFCi][lMIRVRegNamerUtils] Leverage MachineInstrExpressionTrait for hashing a MachineInstr..Dec 12 2019, 10:44 AM
plotfi edited the summary of this revision. (Show Details)
plotfi updated this revision to Diff 233656.Dec 12 2019, 10:45 AM
plotfi edited the summary of this revision. (Show Details)Dec 12 2019, 10:46 AM
plotfi edited the summary of this revision. (Show Details)
plotfi edited the summary of this revision. (Show Details)Dec 12 2019, 10:51 AM
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
128

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.

plotfi added inline comments.Dec 12 2019, 11:19 AM
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
128

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.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
128

Sounds good.

plotfi updated this revision to Diff 233666.Dec 12 2019, 12:24 PM
plotfi retitled this revision from [llvm][NFCi][lMIRVRegNamerUtils] Leverage MachineInstrExpressionTrait for hashing a MachineInstr. to [llvm][NFCi][lMIRVRegNamerUtils] Leverage hash_value for hashing a MachineInstr..
plotfi edited the summary of this revision. (Show Details)

Being more explicit about which MachineOperand types can be hashed and which ones can be skipped due to unstable hashing on pointers as @aditya_nandakumar mentioned. Also brought back hashing on a vreg's defining MI operand because that makes sense.

plotfi marked 3 inline comments as done.Dec 12 2019, 12:26 PM
plotfi marked an inline comment as done.
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
128

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.

llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
76

This feels redundant - I wonder if instead of hashing the opcode (which we already do below), we could hash something unique about the definition (Reg Class/Reg Bank/LLT). That way

x(s32), y(s32) = FOO <Ops>
x(someregclass), y(s64) = FOO <Ops>

the above instructions will hash differently.

plotfi marked an inline comment as done.Dec 12 2019, 2:11 PM
plotfi added inline comments.
llvm/lib/CodeGen/MIRVRegNamerUtils.cpp
76

I think we could hash the regclass/bank along with the opcode. I thought the point of hashing on the opcode directly coming off the def instead of the vreg itself was the main reason the linear hashing technique doesn’t have the ripple effects from previous renaming techniques.

I wonder why the register class isn’t being used for hashing at https://github.com/llvm/llvm-project/blob/6abd01e4624a2c9f8f76e11cc5d57cc7551b5d2a/llvm/lib/CodeGen/MachineOperand.cpp#L347

nhaehnle removed a subscriber: nhaehnle.Dec 13 2019, 5:46 AM
plotfi updated this revision to Diff 233883.Dec 13 2019, 2:19 PM

adding hash against RC/RB

plotfi marked an inline comment as done.Dec 13 2019, 2:19 PM

I'm going to create a new diff that is purely NFC, but moves this code to use the switch-statement. I will add multiple diffs them for specific things like CImm/FPImm. I'll eventually abandon this diff.

plotfi abandoned this revision.Dec 13 2019, 11:51 PM

Abandoning. Will post some piecemeal patches that handle the various MO types separately.