This is expected by instr-remap.test and currently fails when building
LLVM with reverse-iteration: Use std::map instead of SmallDenseMap
which has no guarantees about ordering.
The secondary map 'ProfileData' is needed occasionally (to use cfg hash). doing sorting with std::map is not needed strictly speaking, but it has the nice property of having fixed order.
Sorry, I just saw this now. I had a patch to fix this issue (but I didn't get time to follow-up on it). See D57986.
instr-remap.test is failing because FunctionData is iterated in InstrProfWriter to output function counts, etc. But for two functions with the same name the iteration order is not defined if we use a DenseMap. One way to resolve this is to use an ordered container like MapVector.
Another way to resolve this is to sort FunctionData before iteration. In fact, that is what the reviwers of D57986 suggested.
How about we keep the discussion on the original patch (D57986)? I think we explored the problem space a fair bit there & I'll advocate for the same thing here as I did there: Sorting before emission is likely more efficient than keeping a continuously sorted container, if there's a separate "build" and "iterate" phase you can sort between. At least that's my understanding. std::map and MapVector would both grow memory usage, probably not prohibitively, but a fair bit.
Especially if these are just small clusters of collisions - they can be put in a small container, sorted, emitted, then thrown away - better than changing the whole long-lived/larger data structure.