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

Fri Oct 3 01:07:20 PDT 2014

Hi all,

Thought I should reply on Jiangning's behalf - he's been away due to a
sequence of Chinese national holidays and so probably hasn't seen the
requests to revert. I'll revert on his behalf now.

Cheers,

James

On 3 October 2014 04:17, Eric Christopher <echristo at gmail.com> wrote:

>
> On Oct 2, 2014 6:19 PM, "Bob Wilson" <bob.wilson at apple.com> wrote:
> >
> >
> > > 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.
> >
>
> I think it's probably reasonable to do so now. It sounds like you have
> broad consensus.
>
> -eric
>
> > >
> > > ----- 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
> >
> >
> > _______________________________________________
> > 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141003/212d6e40/attachment.html>