[PATCH] D28312: NewGVN: Fix PR 31501.

[Daniel Berlin via llvm-commits]

On Fri, Jan 6, 2017 at 6:04 PM, Davide Italiano via Phabricator <
reviews at reviews.llvm.org> wrote:

> davide added a comment.
>
> Post commit review, the general idea is obviously correct but few comments
>
>
>
> ================
> Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:393-396
> +  BasicBlock *PHIBlock = I->getParent();
>    auto *PN = cast<PHINode>(I);
> -  auto *E = new (ExpressionAllocator)
> -      PHIExpression(PN->getNumOperands(), I->getParent());
> +  auto *E =
> +      new (ExpressionAllocator) PHIExpression(PN->getNumOperands(),
> PHIBlock);
> ----------------
> The rename is NFC, but, hey, whatever.
>



>
>
> ================
> Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:813-828
> +  // We match the semantics of SimplifyPhiNode from InstructionSimplify
> here.
> +
> +  // See if all arguaments are the same.
> +  // We track if any were undef because they need special handling.
> +  bool HasUndef = false;
> +  auto Filtered = make_filter_range(E->operands(), [&](const Value *Arg)
> {
> +    if (Arg == I)
> ----------------
> Somehow duplicating the logic here is not ideal. I don't expect the
> semantic of `SimplifyPHINode` to change anytime soon, but in case it would,
> we're in a situation where we should keep the two copies in sync.
> I wasn't able to find a nice way to share code between GVN and
> InstSimplify, so this looks good as is.
>
>
> ================
> Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:852
> +      // Only have to check for instructions
> +      if (Instruction *AllSameInst = dyn_cast<Instruction>(AllSameValue))
> +        if (!DT->dominates(AllSameInst, I))
> ----------------
> `auto *`
>
>
> ================
> Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:1870
>  }
> -
>  bool NewGVN::eliminateInstructions(Function &F) {
> ----------------
> Any reason why you removed this newline?
>
> No :(
i'll put it back.

I have clang-format auto-run on save, and sometimes...

>
> ================
> Comment at: llvm/trunk/lib/Transforms/Scalar/NewGVN.cpp:1998-1999
>            }
> -          if (Member && isa<Constant>(Member))
> -            assert(isa<Constant>(CC->RepLeader));
>
> ----------------
> Why this assertion doesn't hold anymore?
>

It is verified directly now.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170106/94025833/attachment.html>