[PATCH] D28117: [NewGVN] Merge unconditional branches before value numbering

[Daniel Berlin via llvm-commits]

I don't think NewGVN should do this, instead of a cleanup pass, unless it
actually produces better analysis results.

I'm already tempted to have us stop wasting time checking
isInstructionTriviallyDead.

I know it matches what GVN does, but that doesn't seem a good reason to me,
and i don't see how it would improve a PRE that properly does dataflow
analysis.
In fact, you can prove it can't :)

(if the branch is unconditional, and can be merged, it's existence should
provably not affect the solution to PRE's dataflow equations).

Does it actually improve elimination?


On Mon, Dec 26, 2016 at 7:33 AM, Davide Italiano via Phabricator <
reviews at reviews.llvm.org> wrote:

> davide updated this revision to Diff 82507.
> davide added a comment.
>
> fix a typo
>
>
> https://reviews.llvm.org/D28117
>
> Files:
>   lib/Transforms/Scalar/NewGVN.cpp
>   test/Transforms/NewGVN/basic.ll
>
>
> Index: test/Transforms/NewGVN/basic.ll
> ===================================================================
> --- test/Transforms/NewGVN/basic.ll
> +++ test/Transforms/NewGVN/basic.ll
> @@ -1,4 +1,3 @@
> -; XFAIL: *
>  ; RUN: opt < %s -newgvn -S | FileCheck %s
>
>  define i32 @main() {
> Index: lib/Transforms/Scalar/NewGVN.cpp
> ===================================================================
> --- lib/Transforms/Scalar/NewGVN.cpp
> +++ lib/Transforms/Scalar/NewGVN.cpp
> @@ -1378,6 +1378,20 @@
>    unsigned ICount = 0;
>    SmallPtrSet<BasicBlock *, 16> VisitedBlocks;
>
> +  // Do a sweep over all the basic blocks to merge unconditional branches.
> +  for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE;) {
> +    BasicBlock *BB = &*FI++;
> +
> +    // In theory here we could pass MemSSA if/when
> MergeBlockIntoPredecessor
> +    // will grow a version which accepts the analysis.
> +    bool removedBlock = MergeBlockIntoPredecessor(
> +        BB, DT, nullptr /* LoopInfo */, nullptr /* MemDep */);
> +
> +    if (removedBlock)
> +      NumGVNBlocksDeleted++;
> +    Changed |= removedBlock;
> +  }
> +
>    // Note: We want RPO traversal of the blocks, which is not quite the
> same as
>    // dominator tree order, particularly with regard whether backedges get
>    // visited first or second, given a block with multiple successors.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161226/56381c16/attachment.html>