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

Bob Wilson bob.wilson at apple.com
Thu Oct 2 18:14:45 PDT 2014 

> On Sep 30, 2014, at 1:22 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> 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

I agree. Jiangning, are you OK with that and if so, will you revert it?

It has been a few days with no response from you, and I want to make sure we don’t forget about this. If you don’t reply soon, I think we should just go ahead and revert it.

> 
> ----- 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
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

More information about the llvm-commits mailing list