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

[Eli Friedman]

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.

-Eli