[llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,

Fri Oct 10 06:31:23 PDT 2014

----- 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>, "Daniel Berlin"
> <dberlin at dberlin.org>
> Sent: Friday, October 10, 2014 3:29:25 AM
> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,
> 
> 
> 
> Hi Hal,
> 
> 
> I'm back!

Hi Jiangning! Welcome back.

> 
> 
> If you have any other empirical evidence, could you please share it?

No, but I think it became clear that there were serious algorithmic concerns, justified by the fact that we needed a cut-off in the first place. The fact remains that, initially, we did not think that this commit would change the asymptotic complexity of the algorithm. I remember thinking that it would only cause additional processing to happen for blocks we had already decided to visit, not cause any additional visits, and so I expected, at most, a constant-factor increase. This turned out to be wrong, and I think that Danny and Nick have done a good job explaining the error in my reasoning.

So at this point we're having a design discussion about a proper solution, and that's great. Nevertheless, the reasoning behind approving the current implementation was that it did not change the complexity, which was wrong, and based on Danny's and Nick's notes, I'm also not convinced that the cut-offs we introduced were proper (meaning minimal). As a matter of general policy, we don't keep commits in tree for which there are serious algorithmic concerns, and so reverting was appropriate.

Please do continue to work on this!

Thanks again,
Hal

> 
> 
> I don't think we should simply revert this solution. At least the
> optimization problem exposed by the test case should be filed into
> bugzilla first. Anyway, I've filed one at
> http://llvm.org/bugs/show_bug.cgi?id=21238 .
> 
> 
> I admit the algorithm itself is a little bit confused, and I may try
> to change the solution. One thought I have is maybe we can
> completely remove "undefined" to see if we still can keep the
> semantic, because in this algorithm undefined will never be
> returned, and it is useless from LVI's users point of view.
> 
> 
> Thanks,
> -Jiangning
> 
> 
> Thanks,
> 
> -Jiangning
> 
> 
> 
> 
> 2014-10-01 4:22 GMT+08:00 Hal Finkel < hfinkel at anl.gov > :
> 
> 
> Jiangning,
> 
> I think that given the algorithmic concerns (and empirical evidence)
> that the enhancement is problematic, I think that we should revert
> it (and the new associated thresholds) while we figure out how to
> solve this properly.
> 
> -Hal
> 
> ----- Original Message -----
> > From: "Daniel Berlin" < dberlin at dberlin.org >
> > To: "Nick Lewycky" < nicholas at mxc.ca >
> > Cc: " llvm-commits at cs.uiuc.edu for LLVM" < llvm-commits at cs.uiuc.edu
> > >
> > Sent: Tuesday, September 30, 2014 2:27:00 PM
> > Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally
> > value on a BB can only be caculated once,
> > 
> > 
> > 
> > FWIW: You don't just lose the performance and memory guarantees if
> > you go back, you actually lose the guarantee that it will terminate
> > 
> > In fact, when someone did something similar to what is being
> > proposed
> > here in a similar GCC algorithm, it caused infinite ping-ponging in
> > some cases until we tracked down what had been committed. While
> > there is a limit in Jiangning's patch, that will just cause it to
> > decide to stop on whatever the current ping pong value is, and
> > completely unrelated changes may cause it to ping pong to the other
> > value.
> > 
> > 
> > 
> > 
> > 
> > 
> > On Tue, Sep 30, 2014 at 12:10 PM, Nick Lewycky < nicholas at mxc.ca >
> > wrote:
> > 
> > 
> > Jiangning Liu wrote:
> > 
> > 
> > Hi Dan,
> > 
> > 2014-09-25 6:01 GMT+08:00 Daniel Berlin < dberlin at dberlin.org
> > <mailto: dberlin at dberlin.org >>:
> > 
> > 
> > 
> > On Wed, Sep 24, 2014 at 12:12 AM, Jiangning Liu
> 
> 
> > < liujiangning1 at gmail.com <mailto: liujiangning1 at gmail. com >>
> > wrote:
> > 
> > Hi Dan,
> > 
> > I consider your question again, and now I think the lattice
> > lowering order in this algorithm should be "overdefined ->
> > constant/constant_range".
> > 
> > At first look, following the text book it seems the lattice
> > value TOP should be "undefined", and BOTTOM should be
> > "overdefined". But this is not true for the specific
> > implementation in this LLVM algorithm.
> > 
> > 
> > Why?
> > 
> > 
> > See my comments below.
> > 
> > 
> > In this algorithm, the lowering order is
> > 
> > Overdefined(TOP) -> constant/constant_range -> Undefined(BOTTOM).
> > 
> > 
> > What does overdefined and undefiend mean then?
> > 
> > 
> > I think "overdefined" means it is a value that can't be known as a
> > constant at compile time, so it might be any value. "undefined"
> > means
> > it
> > is a value we don't care about at all until we evaluate it, so
> > before
> > the algorithm evaluate it, it's value is unknown.
> > 
> > It's a bit stronger than that. Undefined means that it has no
> > value.
> > An uninitialized variable is a good way of thinking about it.
> > 
> > We then collect possible definitions only from statements we've
> > proven are reachable (according to our particular model) and add
> > those definitions. If there are too many definitions, it's
> > overdefined.
> > 
> > 
> > 
> > 
> > 
> > In order to easily implement the algorithm, originally the BBLV
> > is initialized to be BOTTOM, and this doesn't mean the lowering
> > start point is "Undefined", and it should still be "Overdefined"
> > instead.
> > 
> > 
> > If it's initiatlized to bottom, then that is the TOP of the lattice
> > ;)
> > 
> > If we never touch it in the algorithm, it will be kept as it is.
> > 
> > 
> > Right.
> > 
> > This is why once we visit a value at a specific BB, the
> > algorithm will change Undefined to be Overdefined immediately at
> > the very beginning of each lowering procedure.
> > 
> > 
> > It should either be lowered to constant or overdefined.
> > if lowered to constant, it may be lowered again later to
> > overdefined.
> > It should not be lowered to overdefined and then raised back to
> > constant.
> > 
> > 
> > If it is lowering to overdefined too quickly, you should make it
> > *not lower* to overdefined*. When I read the patch, and saw the
> > implementation, it looks like you take *overdefined* values and
> > raise them to *constant* sometimes.
> > Did i misread it?
> > 
> > 
> > I don't think it is to raise to constant, but lower to constant.
> > I'd
> > like to say changing "undefined" to "overdefined" in current
> > algorithm
> > is not a lowering, but an initialization to "overdefined" only.
> > 
> > No, you initialize to "I haven't shown that any definitions are
> > reachable yet" aka. undefined then add more definitions. I think
> > the
> > problem here is when you started trying to connect "undefined" and
> > "overdefined" to "top" and "bottom". There is no standard
> > convention
> > for what "top" and "bottom" mean (or more accurately, there are
> > both
> > conventions). This causes an enormous amount of confusion for
> > academic papers for a start, where even the symbols ⊤ and ⊥ will
> > mean opposite things in different papers, despite being
> > consistently
> > pronounced 'top' and 'bottom' respectively. Just stick with the
> > terms undefined and overdefined. As far as I know, those have crisp
> > meanings.
> > 
> > If a
> > 
> > 
> > value is never visited by the algorithm, it will be "undefined"
> > forever,
> > but this is a meaningless value, and the algorithm never return it.
> > The
> > algorithm can only return either overdefined or
> > constant/constant_range.
> > 
> > It means it's safe to convert into llvm undef, just like an
> > uninitialized variable.
> > 
> > 
> > 
> > If you look into the implementation details of this algorithm,
> > you may find originally the lower ordering is like that.
> > Originally the algorithm will return "overdefined"(TOP) forever
> > for a specific (Val, BB) pair.
> > 
> > 
> > This sounds like either a buggy or conservative implementationt
> > hen,
> > and i would fix *this* issue not by raising overdefined
> > occasionally, but by stopping it from getting to overdefined in the
> > first place.
> > 
> > 
> > Again, Instead, I think it is not raising overdefined, but lowering
> > overdefined. So I don't think it is a bug. I admit there might be
> > some
> > misleading implementation in the algorithm that is not perfect, but
> > conceptually, I personally think it is acceptable, although it
> > doesn't
> > strictly follow the text book.
> > 
> > Whether you call it up or down or left or right doesn't matter. The
> > point is that once you get to overdefined, it doesn't make sense to
> > go back.
> > 
> > Your variables all start with "no definitions", then you only add
> > possible definitions when you show that they're reachable. There's
> > no reason to go back and say "oh wait, just kidding, that one
> > really
> > wasn't possible". That may be correct but it's no longer a lattice
> > driven algorithm and you lose all the performance and memory
> > guarantees that it comes with.
> > 
> > Nick
> > 
> > 
> > 
> > 
> > This could miss some optimization opportunities as I described
> > in the comment. My patch is trying to increase the number of
> > lowering this "overdefined"(TOP) value.
> > 
> > 
> > 
> > This is my current understanding, but maybe I'm wrong.
> > 
> > Thanks,
> > -Jiangning
> > 
> > 
> > 2014-09-23 11:08 GMT+08:00 Jiangning Liu
> > < liujiangning1 at gmail.com <mailto: liujiangning1 at gmail. com >>:
> > 
> > 
> > 
> > Hi Dan,
> > 
> > 
> > So can you explain how you aren't doing this?
> > 
> > It looksl ike you think the lattice goes
> > undefined
> > overdefined
> > constant
> > 
> > That should 100% not be the case
> > the lattice order should be
> > undefined
> > constant
> > overdefined
> > 
> > undefined means you don't know
> > constant means it has one value.
> > overdefined means it has too many values
> > 
> > This is a traditional value range lattice (though
> > sometimes there are more things in the middle).
> > There is no way you should go from overdefined back to
> > constant.
> > 
> > Ah, I see. Thanks for your explanation! I think you are
> > absolutely correct, and I misunderstood lattice value
> > 'overdefined'. The original code is like this,
> > 
> > if ((!BBLV.isUndefined() {
> > ...
> > return;
> > }
> > 
> > // Otherwise, this is the first time we're seeing this
> > block. Reset the
> > // lattice value to overdefined, so that cycles will
> > terminate and be
> > // conservatively correct.
> > BBLV.markOverdefined();
> > 
> > So this algorithm is really conservative. I think the
> > solution might be removing this lowering or adding threshold
> > control for this lowering. Do you have any suggestions?
> > 
> > Thanks,
> > -Jiangning
> > 
> > 
> > 
> > 
> > 
> > 
> > ______________________________ _________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/ mailman/listinfo/llvm-commits
> > 
> > 
> > 
> > _______________________________________________
> > 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