[PATCH] D30154: MemorySSA: Add support for renaming uses in the updater.

[George Burgess IV via llvm-commits]

you can ignore the last "please remove" comment; i skimmed over this last
night and forgot to remove it after davide caught the dump() call. :)

On Sun, Feb 19, 2017 at 11:44 AM, George Burgess IV via Phabricator <
reviews at reviews.llvm.org> wrote:

> george.burgess.iv added a comment.
>
> handful o'nits to supplement piotr's comments, then this LGTM.
>
> thanks!
>
>
>
> ================
> Comment at: include/llvm/Transforms/Utils/MemorySSAUpdater.h:66
>    MemorySSAUpdater(MemorySSA *MSSA) : MSSA(MSSA) {}
> -  void insertDef(MemoryDef *Def);
> +  // Insert a definition into the MemorySSA IR.  RenameUses will rename
> any use
> +  // below the new def block (and any inserted phis).  RenameUses should
> be set
> ----------------
> should this be ///?
>
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1188
> +      // We already visited this during our renaming, which can happen
> when
> +      // being asked to rename multipleblocks. Figure out the incoming
> val,
> +      // which is the last def
> ----------------
> missing space between "multiple blocks"
>
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1191
> +      if (!Visited.insert(BB).second) {
> +        auto *BlockDefs = getWritableBlockDefs(BB);
> +        // Incoming value can only change if there is a block def, and in
> that
> ----------------
> please sink this into the if condition
>
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSA.cpp:1195
> +        if (BlockDefs)
> +          IncomingVal = &*(BlockDefs->rbegin());
> +      } else
> ----------------
> please remove redundant parens
>
>
> ================
> Comment at: lib/Transforms/Utils/MemorySSAUpdater.cpp:299
> +     // incoming value.
> +    if (isa<MemoryDef>(FirstDef))
> +      FirstDef = cast<MemoryDef>(FirstDef)->getDefiningAccess();
> ----------------
> please do `if (auto ... = dyn_cast<MemoryDef>(FirstDef))` instead
>
>
> ================
> Comment at: unittests/Transforms/Utils/MemorySSA.cpp:193
>    MSSA.verifyMemorySSA();
> +  MSSA.dump();
>  }
> ----------------
> davide wrote:
> > IIRC `dump()` is supposed to be used only in a debugger and `print()`
> should be used instead, but I might be wrong.
> please remove
>
>
> https://reviews.llvm.org/D30154
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170219/9dcd3908/attachment.html>