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

[Julien Lerouge]

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;

That being said, I can add more comment if that helps.

Thanks,
Julien

-- 
Julien Lerouge
PGP Key Id: 0xB1964A62
PGP Fingerprint: 392D 4BAD DB8B CE7F 4E5F FA3C 62DB 4AA7 B196 4A62
PGP Public Key from: keyserver.pgp.com