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

[Hal Finkel]

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