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

Thu Oct 2 20:17:17 PDT 2014

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141002/1cca7af9/attachment.html>