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

[Jiangning Liu]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140730/f0e08f5a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_lazy_value_info_2.patch
Type: text/x-patch
Size: 2819 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140730/f0e08f5a/attachment.bin>