[PATCH] Fix lazy value info computation to improve jump threading for a common case

[Hal Finkel]

----- Original Message -----
> From: "Jiangning Liu" <liujiangning1 at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>, "Nick Lewycky" <nicholas at mxc.ca>
> Sent: Thursday, July 31, 2014 10:24:15 PM
> Subject: Re: [PATCH] Fix lazy value info computation to improve jump threading for a common case
> 
> 
> Hi Hal,
> 
> 
> Thanks for your comments!
> 
> 
> I don't have a precised compile-time test infrastructure, so I can
> only use my local box to try. And if you have a precised
> compile-time test infrastructure, could you please help me to have a
> try?

If you run the LLVM test suite, it will also collect compile-time numbers. That's normally what I use, run a few times in each configuration.

> 
> 
> My local box is Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz, and I tried
> to build spec2000 int with a single core, so now I got the following
> result,
> 
> 
> 
> 
> Without my patch:
> 
> 
> real 1m28.809s
> user 1m19.817s
> sys 0m6.158s
> 
> 
> real 1m27.538s
> user 1m19.726s
> sys 0m6.083s
> 
> 
> real 1m27.551s
> user 1m19.710s
> sys 0m6.012s
> 
> 
> With my patch:
> 
> 
> real 1m27.935s
> user 1m20.250s
> sys 0m6.046s
> 
> 
> real 1m27.867s
> user 1m20.035s
> sys 0m6.268s
> 
> 
> real 1m27.511s
> user 1m19.936s
> sys 0m6.266s
> 
> 
> I don't really see compile-time change at all.

If there is a an effect here, it seems definitely no larger than 1%.

In this case, since we're checking a particular pass or two, you can compile with -mllvm -stats and look specifically at the time taken by Jump Threading and Value Propagation.

 -Hal

> 
> 
> Thanks,
> -Jiangning
> 
> 
> 
> 
> 
> 2014-08-01 0:02 GMT+08:00 Hal Finkel < hfinkel at anl.gov > :
> 
> 
> Did you measure the compile-time impact of this change? I suspect
> this will be fine because, while it can increase the total amount of
> work done over all visits to a block, it does not increase the total
> number of visits (so the asymptotic complexity bound is still the
> same). Nevertheless, we should double-check. If there is nothing
> significant, then this LGTM.
> 
> Thanks,
> Hal
> 
> 
> 
> ----- Original Message -----
> > From: "Jiangning Liu" < liujiangning1 at gmail.com >
> > To: "Nick Lewycky" < nicholas at mxc.ca >
> > Cc: " llvm-commits at cs.uiuc.edu for LLVM" < llvm-commits at cs.uiuc.edu
> > >
> > Sent: Wednesday, July 30, 2014 4:57:14 AM
> > Subject: Re: [PATCH] Fix lazy value info computation to improve
> > jump threading for a common case
> > 
> > 
> > 
> > Hi Nick,
> > 
> > 
> > 
> > 2014-07-30 16:15 GMT+08:00 Nick Lewycky < nicholas at mxc.ca > :
> > 
> > 
> > 
> > Jiangning Liu wrote:
> > 
> > 
> > Hi,
> > 
> > Attached patch is to fix an issue in lazy value info computation,
> > and
> > finally it can improve jump threading for a common case.
> > 
> > Lazy value info computation algorithm tries to use lattice to
> > propagate
> > the given value through control flow. The problem of the algorithm
> > is
> > the basic block can only be visited once. Once the lattice state is
> > changed from "undefined" to other states, the lattice state will be
> > cached, and won't be able to be changed any longer.
> > 
> > For the following simple control flow graph like below,
> > 
> > BB1->BB2, BB1->BB3, BB2->BB3, BB2->BB4
> > 
> > When computing a given value on edge BB2->BB3, if B2 doesn't have
> > that
> > value info at all, the algorithm will try to ask for information
> > from
> > BB2's predecessors, and then return to BB2 again to merge the info
> > from
> > predecessors, so BB2 will be visited twice. Unfortunately, at first
> > visit of BB2, the lattice state will be changed from "undefined" to
> > "overdefined", and then it will not be able to be changed any
> > longer.
> > 
> > That sounds correct. Once overdefined, it's stuck and can't be
> > changed.
> > 
> > How did it get marked overdefined in the first place?
> > 
> > 
> > it is marked as overdefined immediately after the if statement I
> > modified.
> > 
> > 
> > Did we speculatively mark it overdefined in case we discovered that
> > the computation is in terms of ourselves through a phi node or by
> > directly depending on ourselves (only possibly if our CFG subgraph
> > is not reachable from entry)? Why could we mark it undefined
> > instead?
> > 
> > 
> > The initialization is marked as undefined, I think. Lattice is
> > trying
> > to lower the value in this order undefined->overdefined->...
> > 
> > 
> > The algorithm is so-called "lazy", so at the beginning the value
> > doesn't exist for the block, and it is just initialized as
> > undefined
> > at the first time the algorithm visits it.
> > 
> > 
> > Why couldn't we add a new "non-local-lookup-in-progress- here!"
> > state
> 
> 
> > instead?
> > 
> > 
> > I think IsOverdefined already implies non-local-lookup, it would be
> > "undefined" otherwise.
> > 
> > 
> > 
> > 
> > 
> > 
> > This patch is to simply check "overdefined", and make sure the
> > algorithm
> > can still move on to propagate the values from predecessor edges to
> > the
> > block itself.
> > 
> > That sounds like it could be really bad for compile-time
> > performance.
> > 
> 
> 
> 
> > 
> > 
> > 
> > The performance experiment for spec shows pretty good result on
> > aarch64
> > for this fix, and the 253_perlbmk can even have >5% performance
> > improvement.
> > 
> > + // we need to compute it's value again.
> > 
> > Typo, "it's" -> "its".
> > 
> > + // check BB2 again, and at this momement BB2 has Overdefined
> > value
> > for %v in
> > 
> > Typo, "momement" -> "moment".
> > 
> > + // BB2. So we should have to follow data flow propagation
> > algirithm
> > to get the
> > 
> > Typo, "algirithm" -> "algorithm".
> > 
> > 
> > 
> > So for so many typos. Attached is the updated version.
> > 
> > 
> > 
> > BTW, there are some unreviewed patches for lazy value info here:
> > http://lists.cs.uiuc.edu/ pipermail/llvm-commits/Week-
> > of-Mon-20140127/203418.html . The problem description sounds like
> > it
> > could be related, it's not obviously not the same problem to me.
> > Did
> > his patch come with tests? Does your patch fix those tests too?
> > 
> > 
> > I tried Olivier's test case in the 4th patch, and my patch can't
> > really solve his issue, I think.
> > 
> > 
> > I think Olivier was pursuing an even complete fix for precision
> > purpose. As Nuno pointed out LVI is not designed as recursive on
> > purpose, so there must be some precision lost. For example, my test
> > case can be even extended with more incoming edges for BB2 to
> > influence the value, but the algorithm will stop as soon as it
> > finds
> > the value is neither undefined nor overdefined, so we will not have
> > chance to narrow down the value further.
> > 
> > 
> > Thanks,
> > -Jiangning
> > 
> > 
> > 
> > 
> > 
> > Nick
> > 
> > 
> > 
> > _______________________________________________
> > 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