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

[Hal Finkel]

----- Original Message -----
> From: "Jonas Paulsson" <jonas.paulsson at ericsson.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu, "Owen Anderson" <owen at apple.com>
> Sent: Wednesday, February 11, 2015 2:13:50 AM
> Subject: RE: [DAGCombiner] Poor DAG combining with alias analysis turned on.
> 
> 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.

Okay, with that, LGTM. Let's try it. (I have some suspicion that the original comment about not adding to the work list was intended to avoid some kind of infinite loop, at least when it was originally written, but I don't see why that should be the case, so we'll see...)

 -Hal

> 
> /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
> 

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