[llvm-commits] [llvm] r149838 - in /llvm/trunk: lib/Transforms/Scalar/GVN.cpp test/Transforms/GVN/condprop.ll

[Eli Friedman]

On Sun, Feb 5, 2012 at 10:25 AM, Duncan Sands <baldrick at free.fr> wrote:
> Author: baldrick
> Date: Sun Feb  5 12:25:50 2012
> New Revision: 149838
>
> URL: http://llvm.org/viewvc/llvm-project?rev=149838&view=rev
> Log:
> Reduce the number of dom queries made by GVN's conditional propagation
> logic by half: isOnlyReachableViaThisEdge was trying to be clever and
> handle the case of a branch to a basic block which is contained in a
> loop.  This costs a domtree lookup and is completely useless due to
> GVN's position in the pass pipeline: all loops have preheaders at this
> point, which means it is enough for isOnlyReachableViaThisEdge to check
> that Dst has only one predecessor.  (I checked this theoretical argument
> by running over the entire nightly testsuite, and indeed it is so!).
>
> Modified:
>    llvm/trunk/lib/Transforms/Scalar/GVN.cpp
>    llvm/trunk/test/Transforms/GVN/condprop.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=149838&r1=149837&r2=149838&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Sun Feb  5 12:25:50 2012
> @@ -1994,37 +1994,15 @@
>  /// particular 'Dst' must not be reachable via another edge from 'Src'.
>  static bool isOnlyReachableViaThisEdge(BasicBlock *Src, BasicBlock *Dst,
>                                        DominatorTree *DT) {
> -  // First off, there must not be more than one edge from Src to Dst, there
> -  // should be exactly one.  So keep track of the number of times Src occurs
> -  // as a predecessor of Dst and fail if it's more than once.
> -  bool SawEdgeFromSrc = false;
> -  for (pred_iterator PI = pred_begin(Dst), PE = pred_end(Dst); PI != PE; ++PI) {
> -    if (*PI != Src)
> -      continue;
> -    // An edge from Src to Dst.
> -    if (SawEdgeFromSrc)
> -      // There are multiple edges from Src to Dst - fail.
> -      return false;
> -    SawEdgeFromSrc = true;
> -  }
> -  assert(SawEdgeFromSrc && "No edge between these basic blocks!");
> -
> -  // Secondly, any other predecessors of Dst should be dominated by Dst.  If the
> -  // predecessor is not dominated by Dst, then it must be possible to reach it
> -  // either without passing through Src (thus not via the edge) or by passing
> -  // through Src but taking a different edge out of Src.  Either way Dst can be
> -  // reached without passing via the edge, so fail.
> -  for (pred_iterator PI = pred_begin(Dst), PE = pred_end(Dst); PI != PE; ++PI) {
> -    BasicBlock *Pred = *PI;
> -    if (Pred != Src && !DT->dominates(Dst, Pred))
> -      return false;
> -  }
> -
> -  // Every path from the entry block to Dst must at some point pass to Dst from
> -  // a predecessor that is not dominated by Dst.  This predecessor can only be
> -  // Src, since all others are dominated by Dst.  As there is only one edge from
> -  // Src to Dst, the path passes by this edge.
> -  return true;
> +  // While in theory it is interesting to consider the case in which Dst has
> +  // more than one predecessor, because Dst might be part of a loop which is
> +  // only reachable from Src, in practice it is pointless since at the time
> +  // GVN runs all such loops have preheaders, which means that Dst will have
> +  // been changed to have only one predecessor, namely Src.
> +  pred_iterator PI = pred_begin(Dst), PE = pred_end(Dst);
> +  assert(PI != PE && *PI == Src && "No edge between these basic blocks!");
> +  (void)Src;
> +  return PE == ++PI;
>  }

I don't think the assertion is actually asserting what you want it to;
it's checking that Src is *the first* predecessor of Dst, not just
that it's *a* predecessor.

-Eli