[PATCH] D39593: [ADCE] Use MapVector for BlockInfo to make iteration order deterministic

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:214
 void AggressiveDeadCodeElimination::initialize() {
-  auto NumBlocks = F.size();
-
-  // We will have an entry in the map for each block so we grow the
-  // structure to twice that size to keep the load factor low in the hash table.
-  BlockInfo.reserve(NumBlocks);
   size_t NumInsts = 0;
 
----------------
kuhar wrote:
> uabelho wrote:
> > kuhar wrote:
> > > I can see that MapVector doesn't expose `.reserve` -- what is the reason for that? While I don't claim that it would have any noticeable performance impact here, I'm a bit surprised by that.
> > Unfortunately I have no idea. I just noticed there is no such function, and thus removed the call.
> > 
> > I don't really understand the comment preceding the original reserve call though, it says "so we grow the structure to twice that size", but then it only reserves NumBlocks anyway? Not 2*Numblocks?
> I think it was meant to be `.reserve(NumBlocks * 2)`, but it also makes sense to just reduce the number of allocations.
> 
> I looked at MapVector and it should very easy to add reserve to it. Then, I think it would make sense to keep it here, and to update the comment or size for whichever makes more sense.
Yes, you're right. I created a reserve method now. I left the original reserve code and comment in ADCE
in place since I don't know what makes most sense, and also, fixing the amout we reserve is not the focus of this patch anyway.


https://reviews.llvm.org/D39593