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

[Hal Finkel]

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