[DAGCombiner] Poor DAG combining with alias analysis turned on.

[Jonas Paulsson]

Hi Hal,

> Why don't we want to add TF users to the worklist always? Is it generically useless unless AA is used?

visitSTORE() and visitLOAD()  will call FindBetterChain() only if AA is used, and that function will search
upwards through chain edges as long as AA says 'no-alias'. A 'BetterChain' is then returned, and the
dependency is broken by introducing a new token factor:

visitSTORE() called on Store1:

                          BetterChain
    Store0                  ^                Store0    
        ^           =>        |                     ^
        |                     Store1              |
    Store1                     \                /
                                  Token factor

In DAGs with many memory accesses that AA is successful with, quite a lot of token factors are generated,
and an enormous amount of edges that connects them might result.
 
So, I would say that the need for adding TF users (and the new TF) is absolutely true with AA, but without it
I really don't know. It depends on how token factors are used in places that do not depend on AA.

>If nothing else, this comment needs to explain why.

How about:
// Add users to worklist if AA is enabled, since it may introduce a lot of new chained token factors while
// removing memory dependencies.

/Jonas
 
-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov] 
Sent: den 10 februari 2015 20:06
To: Jonas Paulsson
Cc: llvm-commits at cs.uiuc.edu; Owen Anderson
Subject: Re: [DAGCombiner] Poor DAG combining with alias analysis turned on.

Hi Jonas,

Why don't we want to add TF users to the worklist always? Is it generically useless unless AA is used?

> + // Don't add users to work list, unless alias analysis is used.

If nothing else, this comment needs to explain why.

 -Hal

----- Original Message -----
> From: "Jonas Paulsson" <jonas.paulsson at ericsson.com>
> To: llvm-commits at cs.uiuc.edu
> Sent: Monday, February 9, 2015 9:38:18 AM
> Subject: [DAGCombiner] Poor DAG combining with alias analysis turned on.
> 
> Hi,
> 
> I have a test case with an unrolled loop body, consisting of a long 
> sequence of non-overlapping stores. I found that with alias analysis 
> it became very compile time expensive (53 seconds, without AA it was
> 0.02 sec). The program spent a lot of time in WalkChainUsers() (isel), 
> and it was due to all the token factor nodes introduced by the 
> combiner.
> 
> 
> 
> I spent some time looking into it, and found that nearly all of the 
> compile time disappeared if the combiner added a new token factor node 
> to the worklist. The case was that there was a mess of token factors 
> at the bottom of the DAG, that disappeared after reiteration over 
> those nodes.
> 
> 
> 
> Test case supplied, to run: llc -mtriple=arm -combiner-alias-analysis 
> unrolledloop.opt.ll
> 
> 
> 
> I supply a patch for this. Let me know if this seems reasonable to use 
> and if I can commit.
> 
> 
> 
> /Jonas Paulsson
> 
> 
> 
> 
> 
> Patch:
> 
> 
> 
> From 0123157edefc2aea3137b702b1fa24ebd41e7709 Mon Sep 17 00:00:00
> 2001
> 
> From: Jonas Paulsson <jonas.paulsson at ericsson.com>
> 
> Date: Mon, 9 Feb 2015 16:19:53 +0100
> 
> Subject: [PATCH] Fix SelectionDAG compile time issue with alias 
> analysis.
> 
> 
> 
> Add new token factor node to worklist if alias analysis is turned on,
> 
> in DAGCombiner::visitTokenFactor().
> 
> ---
> 
> lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 6 ++++--
> 
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> 
> 
> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> 
> index 8a4b602..dbf6ef7 100644
> 
> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> 
> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> 
> @@ -1539,8 +1539,10 @@ SDValue DAGCombiner::visitTokenFactor(SDNode
> *N) {
> 
> Result = DAG.getNode(ISD::TokenFactor, SDLoc(N), MVT::Other, Ops);
> 
> }
> 
> 
> 
> - // Don't add users to work list.
> 
> - return CombineTo(N, Result, false);
> 
> + // Don't add users to work list, unless alias analysis is used.
> 
> + bool UseAA = CombinerAA.getNumOccurrences() > 0 ? CombinerAA
> 
> + : DAG.getSubtarget().useAA();
> 
> + return CombineTo(N, Result, UseAA /*add to worklist*/);
> 
> }
> 
> 
> 
> return Result;
> 
> --
> 
> 1.8.4.2
> 
> 
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory