[llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,
Hal Finkel
hfinkel at anl.gov
Fri Oct 3 04:42:35 PDT 2014
----- Original Message -----
> From: "James Molloy" <james at jamesmolloy.co.uk>
> To: "Eric Christopher" <echristo at gmail.com>
> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>
> Sent: Friday, October 3, 2014 4:42:56 AM
> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,
>
>
>
> Reverted in r218971.
Did you revert r218231 also? I think we should back that out too (its only motivation was an attempted fix of r215343).
Thanks again,
Hal
>
>
> On 3 October 2014 09:07, James Molloy < james at jamesmolloy.co.uk >
> wrote:
>
>
>
> 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
>
>
>
>
> _______________________________________________
> 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
More information about the llvm-commits
mailing list