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

James Molloy james at jamesmolloy.co.uk
Fri Oct 3 02:42:56 PDT 2014


Reverted in r218971.

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


More information about the llvm-commits mailing list