[polly] r182655 - fix insertion of values in BBMap

[David Blaikie]

On Fri, May 24, 2013 at 10:16 AM, Sebastian Pop <spop at codeaurora.org> wrote:

> Author: spop
> Date: Fri May 24 12:16:02 2013
> New Revision: 182655
>
> URL: http://llvm.org/viewvc/llvm-project?rev=182655&view=rev
> Log:
> fix insertion of values in BBMap
>
> In GDB when "step" through generateScalarLoad and "finish" the call, the
> returned value is non NULL, however when printing the value contained in
> BBMap[Load] after this stmt:
>
>   BBMap[Load] = generateScalarLoad(...);
>
> the value in BBMap[Load] is NULL, and the BBMap.count(Load) is 1.
>
> The only intuitive idea that I have to explain this behavior is that we are
> playing with the undefined behavior of eval order of the params for the
> function
> standing for "BBMap[Load] = generateScalarLoad()". "BBMap[Load] = " may be
> executed before generateScalarLoad is called.
>
> Here are some other possible explanations from Will Dietz <w at wdtz.org>:
>
> The error is likely due to BBMap[Load] being evaluated first (creating
> a {Load -> uninitialized } entry in the DenseMap), then
> generateScalarLoad eventually accesses the same element and finds it
> to be NULL (DenseMap[Old])..  Offhand I'm not sure if this is
> guaranteed to be NULL or if it's uninitialized and happens to be NULL.
>

These seem like the same explanation, but yes - order of evaluation of the
LHS and RHS are unspecified.


>
> The same issue can also go wrong in an even worse way: the second
> DenseMap access can trigger a rehash and *invalidate* the an earlier
> evaluated expression (for example LHS of the assignment), leading to a
> crash when performing the assignment store.
>
> Modified:
>     polly/trunk/lib/CodeGen/BlockGenerators.cpp
>
> Modified: polly/trunk/lib/CodeGen/BlockGenerators.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/polly/trunk/lib/CodeGen/BlockGenerators.cpp?rev=182655&r1=182654&r2=182655&view=diff
>
> ==============================================================================
> --- polly/trunk/lib/CodeGen/BlockGenerators.cpp (original)
> +++ polly/trunk/lib/CodeGen/BlockGenerators.cpp Fri May 24 12:16:02 2013
> @@ -185,6 +185,7 @@ Value *BlockGenerator::getNewValue(const
>    }
>
>    if (BBMap.count(Old)) {
> +    assert(BBMap[Old] && "BBMap[Old] should not be NULL!");
>      return BBMap[Old];
>    }
>
> @@ -354,12 +355,14 @@ void BlockGenerator::copyInstruction(con
>      return;
>
>    if (const LoadInst *Load = dyn_cast<LoadInst>(Inst)) {
> -    BBMap[Load] = generateScalarLoad(Load, BBMap, GlobalMap, LTS);
> +    Value *NewLoad = generateScalarLoad(Load, BBMap, GlobalMap, LTS);
> +    BBMap[Load] = NewLoad;
>      return;
>    }
>
>    if (const StoreInst *Store = dyn_cast<StoreInst>(Inst)) {
> -    BBMap[Store] = generateScalarStore(Store, BBMap, GlobalMap, LTS);
> +    Value *NewStore = generateScalarStore(Store, BBMap, GlobalMap, LTS);
> +    BBMap[Store] = NewStore;
>

Perhaps you could add a comment to these two blocks to explain why there's
an extra variable here? Or you could use insert:

BBMap.insert(std::make_pair(Store, generateScalarStore(...)));


>      return;
>    }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130524/95c11b10/attachment.html>