[llvm-commits] [llvm] r166435 - /llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp

[Chandler Carruth]

On Mon, Oct 22, 2012 at 1:25 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Mon, Oct 22, 2012 at 1:24 PM, Julien Lerouge <jlerouge at apple.com> wrote:
>> On Mon, Oct 22, 2012 at 01:06:38PM -0700, Eli Friedman wrote:
>>> On Mon, Oct 22, 2012 at 12:43 PM, Julien Lerouge <jlerouge at apple.com> wrote:
>>> > Author: jlerouge
>>> > Date: Mon Oct 22 14:43:56 2012
>>> > New Revision: 166435
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=166435&view=rev
>>> > Log:
>>> > Iterating over a DenseMap<std::pair<BasicBlock*, unsigned>, PHINode*> is not
>>> > deterministic, replace it with a DenseMap<std::pair<unsigned, unsigned>,
>>> > PHINode*> (we already have a map from BasicBlock to unsigned).
>>> >
>>> > <rdar://problem/12541389>
>>> >
>>> > Modified:
>>> >     llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
>>> >
>>> > Modified: llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
>>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp?rev=166435&r1=166434&r2=166435&view=diff
>>> > ==============================================================================
>>> > --- llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp (original)
>>> > +++ llvm/trunk/lib/Transforms/Utils/PromoteMemoryToRegister.cpp Mon Oct 22 14:43:56 2012
>>> > @@ -214,7 +214,7 @@
>>> >
>>> >      /// NewPhiNodes - The PhiNodes we're adding.
>>> >      ///
>>> > -    DenseMap<std::pair<BasicBlock*, unsigned>, PHINode*> NewPhiNodes;
>>> > +    DenseMap<std::pair<unsigned, unsigned>, PHINode*> NewPhiNodes;
>>> >
>>> >      /// PhiToAllocaMap - For each PHI node, keep track of which entry in Allocas
>>> >      /// it corresponds to.
>>> > @@ -588,7 +588,7 @@
>>> >    while (EliminatedAPHI) {
>>> >      EliminatedAPHI = false;
>>> >
>>> > -    for (DenseMap<std::pair<BasicBlock*, unsigned>, PHINode*>::iterator I =
>>> > +    for (DenseMap<std::pair<unsigned, unsigned>, PHINode*>::iterator I =
>>> >             NewPhiNodes.begin(), E = NewPhiNodes.end(); I != E;) {
>>> >        PHINode *PN = I->second;
>>>
>>> It would be much clearer that this is deterministic if you used
>>> llvm::MapVector instead.
>>>
>>> -Eli
>>
>> MapVector doesn't support removing elements, so the code would have to
>> change a lot more.
>>
>> Also, there is already a map from Basic Block to unsigned for that
>> explicit purpose so it's more efficient to just re-use it I think:
>>
>>     /// BBNumbers - Contains a stable numbering of basic blocks to avoid
>>     /// non-determinstic behavior.
>>     DenseMap<BasicBlock*, unsigned> BBNumbers;
>
> OK.
>
>> That being said, I can add more comment if that helps.
>
> Yes, please: iterating over a DenseMap is automatically suspicious.

Worse, it's not guaranteed. The contract of the hashing used by
DenseMap explicitly says code should not depend on it remaining
deterministic. We'd like to make it aggressively non-deterministic to
flush these types of problems out more quickly. There is essentially
*always* a better option than a hashtable for deterministic iteration.

In this case, the BBNumbers are a dense, 0-based counting of basic
blacks. Why use a hashtable at all? Why not just store a vector of
blocks and use the unsigned value as in index? That is their common
usage.

I would ask that any time we have iteration-on-densemap we consider it a bug.

If we need to teach MapVector to support more operations, that can be
done. The strategy is always to be lazy with such datastructures,
adding interfaces as needed.