<p dir="ltr"><br>
On Oct 2, 2014 6:19 PM, "Bob Wilson" <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:<br>
><br>
><br>
> > On Sep 30, 2014, at 1:22 PM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:<br>
> ><br>
> > Jiangning,<br>
> ><br>
> > 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.<br>
> ><br>
> > -Hal<br>
><br>
> I agree. Jiangning, are you OK with that and if so, will you revert it?<br>
><br>
> 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.<br>
></p>
<p dir="ltr">I think it's probably reasonable to do so now. It sounds like you have broad consensus. </p>
<p dir="ltr">-eric<br></p>
<p dir="ltr">> ><br>
> > ----- Original Message -----<br>
> >> From: "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>><br>
> >> To: "Nick Lewycky" <<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>><br>
> >> Cc: "<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> for LLVM" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>><br>
> >> Sent: Tuesday, September 30, 2014 2:27:00 PM<br>
> >> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,<br>
> >><br>
> >><br>
> >><br>
> >> FWIW: You don't just lose the performance and memory guarantees if<br>
> >> you go back, you actually lose the guarantee that it will terminate<br>
> >><br>
> >> In fact, when someone did something similar to what is being proposed<br>
> >> here in a similar GCC algorithm, it caused infinite ping-ponging in<br>
> >> some cases until we tracked down what had been committed. While<br>
> >> there is a limit in Jiangning's patch, that will just cause it to<br>
> >> decide to stop on whatever the current ping pong value is, and<br>
> >> completely unrelated changes may cause it to ping pong to the other<br>
> >> value.<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> On Tue, Sep 30, 2014 at 12:10 PM, Nick Lewycky < <a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a> ><br>
> >> wrote:<br>
> >><br>
> >><br>
> >> Jiangning Liu wrote:<br>
> >><br>
> >><br>
> >> Hi Dan,<br>
> >><br>
> >> 2014-09-25 6:01 GMT+08:00 Daniel Berlin < <a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a><br>
> >> <mailto: <a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a> >>:<br>
> >><br>
> >><br>
> >><br>
> >> On Wed, Sep 24, 2014 at 12:12 AM, Jiangning Liu<br>
> >> < <a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a> <mailto: liujiangning1@gmail. com >> wrote:<br>
> >><br>
> >> Hi Dan,<br>
> >><br>
> >> I consider your question again, and now I think the lattice<br>
> >> lowering order in this algorithm should be "overdefined -><br>
> >> constant/constant_range".<br>
> >><br>
> >> At first look, following the text book it seems the lattice<br>
> >> value TOP should be "undefined", and BOTTOM should be<br>
> >> "overdefined". But this is not true for the specific<br>
> >> implementation in this LLVM algorithm.<br>
> >><br>
> >><br>
> >> Why?<br>
> >><br>
> >><br>
> >> See my comments below.<br>
> >><br>
> >><br>
> >> In this algorithm, the lowering order is<br>
> >><br>
> >> Overdefined(TOP) -> constant/constant_range -> Undefined(BOTTOM).<br>
> >><br>
> >><br>
> >> What does overdefined and undefiend mean then?<br>
> >><br>
> >><br>
> >> I think "overdefined" means it is a value that can't be known as a<br>
> >> constant at compile time, so it might be any value. "undefined" means<br>
> >> it<br>
> >> is a value we don't care about at all until we evaluate it, so before<br>
> >> the algorithm evaluate it, it's value is unknown.<br>
> >><br>
> >> It's a bit stronger than that. Undefined means that it has no value.<br>
> >> An uninitialized variable is a good way of thinking about it.<br>
> >><br>
> >> We then collect possible definitions only from statements we've<br>
> >> proven are reachable (according to our particular model) and add<br>
> >> those definitions. If there are too many definitions, it's<br>
> >> overdefined.<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> In order to easily implement the algorithm, originally the BBLV<br>
> >> is initialized to be BOTTOM, and this doesn't mean the lowering<br>
> >> start point is "Undefined", and it should still be "Overdefined"<br>
> >> instead.<br>
> >><br>
> >><br>
> >> If it's initiatlized to bottom, then that is the TOP of the lattice<br>
> >> ;)<br>
> >><br>
> >> If we never touch it in the algorithm, it will be kept as it is.<br>
> >><br>
> >><br>
> >> Right.<br>
> >><br>
> >> This is why once we visit a value at a specific BB, the<br>
> >> algorithm will change Undefined to be Overdefined immediately at<br>
> >> the very beginning of each lowering procedure.<br>
> >><br>
> >><br>
> >> It should either be lowered to constant or overdefined.<br>
> >> if lowered to constant, it may be lowered again later to overdefined.<br>
> >> It should not be lowered to overdefined and then raised back to<br>
> >> constant.<br>
> >><br>
> >><br>
> >> If it is lowering to overdefined too quickly, you should make it<br>
> >> *not lower* to overdefined*. When I read the patch, and saw the<br>
> >> implementation, it looks like you take *overdefined* values and<br>
> >> raise them to *constant* sometimes.<br>
> >> Did i misread it?<br>
> >><br>
> >><br>
> >> I don't think it is to raise to constant, but lower to constant. I'd<br>
> >> like to say changing "undefined" to "overdefined" in current<br>
> >> algorithm<br>
> >> is not a lowering, but an initialization to "overdefined" only.<br>
> >><br>
> >> No, you initialize to "I haven't shown that any definitions are<br>
> >> reachable yet" aka. undefined then add more definitions. I think the<br>
> >> problem here is when you started trying to connect "undefined" and<br>
> >> "overdefined" to "top" and "bottom". There is no standard convention<br>
> >> for what "top" and "bottom" mean (or more accurately, there are both<br>
> >> conventions). This causes an enormous amount of confusion for<br>
> >> academic papers for a start, where even the symbols ⊤ and ⊥ will<br>
> >> mean opposite things in different papers, despite being consistently<br>
> >> pronounced 'top' and 'bottom' respectively. Just stick with the<br>
> >> terms undefined and overdefined. As far as I know, those have crisp<br>
> >> meanings.<br>
> >><br>
> >> If a<br>
> >><br>
> >><br>
> >> value is never visited by the algorithm, it will be "undefined"<br>
> >> forever,<br>
> >> but this is a meaningless value, and the algorithm never return it.<br>
> >> The<br>
> >> algorithm can only return either overdefined or<br>
> >> constant/constant_range.<br>
> >><br>
> >> It means it's safe to convert into llvm undef, just like an<br>
> >> uninitialized variable.<br>
> >><br>
> >><br>
> >><br>
> >> If you look into the implementation details of this algorithm,<br>
> >> you may find originally the lower ordering is like that.<br>
> >> Originally the algorithm will return "overdefined"(TOP) forever<br>
> >> for a specific (Val, BB) pair.<br>
> >><br>
> >><br>
> >> This sounds like either a buggy or conservative implementationt hen,<br>
> >> and i would fix *this* issue not by raising overdefined<br>
> >> occasionally, but by stopping it from getting to overdefined in the<br>
> >> first place.<br>
> >><br>
> >><br>
> >> Again, Instead, I think it is not raising overdefined, but lowering<br>
> >> overdefined. So I don't think it is a bug. I admit there might be<br>
> >> some<br>
> >> misleading implementation in the algorithm that is not perfect, but<br>
> >> conceptually, I personally think it is acceptable, although it<br>
> >> doesn't<br>
> >> strictly follow the text book.<br>
> >><br>
> >> Whether you call it up or down or left or right doesn't matter. The<br>
> >> point is that once you get to overdefined, it doesn't make sense to<br>
> >> go back.<br>
> >><br>
> >> Your variables all start with "no definitions", then you only add<br>
> >> possible definitions when you show that they're reachable. There's<br>
> >> no reason to go back and say "oh wait, just kidding, that one really<br>
> >> wasn't possible". That may be correct but it's no longer a lattice<br>
> >> driven algorithm and you lose all the performance and memory<br>
> >> guarantees that it comes with.<br>
> >><br>
> >> Nick<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> This could miss some optimization opportunities as I described<br>
> >> in the comment. My patch is trying to increase the number of<br>
> >> lowering this "overdefined"(TOP) value.<br>
> >><br>
> >><br>
> >><br>
> >> This is my current understanding, but maybe I'm wrong.<br>
> >><br>
> >> Thanks,<br>
> >> -Jiangning<br>
> >><br>
> >><br>
> >> 2014-09-23 11:08 GMT+08:00 Jiangning Liu<br>
> >> < <a href="mailto:liujiangning1@gmail.com">liujiangning1@gmail.com</a> <mailto: liujiangning1@gmail. com >>:<br>
> >><br>
> >><br>
> >><br>
> >> Hi Dan,<br>
> >><br>
> >><br>
> >> So can you explain how you aren't doing this?<br>
> >><br>
> >> It looksl ike you think the lattice goes<br>
> >> undefined<br>
> >> overdefined<br>
> >> constant<br>
> >><br>
> >> That should 100% not be the case<br>
> >> the lattice order should be<br>
> >> undefined<br>
> >> constant<br>
> >> overdefined<br>
> >><br>
> >> undefined means you don't know<br>
> >> constant means it has one value.<br>
> >> overdefined means it has too many values<br>
> >><br>
> >> This is a traditional value range lattice (though<br>
> >> sometimes there are more things in the middle).<br>
> >> There is no way you should go from overdefined back to<br>
> >> constant.<br>
> >><br>
> >> Ah, I see. Thanks for your explanation! I think you are<br>
> >> absolutely correct, and I misunderstood lattice value<br>
> >> 'overdefined'. The original code is like this,<br>
> >><br>
> >> if ((!BBLV.isUndefined() {<br>
> >> ...<br>
> >> return;<br>
> >> }<br>
> >><br>
> >> // Otherwise, this is the first time we're seeing this<br>
> >> block. Reset the<br>
> >> // lattice value to overdefined, so that cycles will<br>
> >> terminate and be<br>
> >> // conservatively correct.<br>
> >> BBLV.markOverdefined();<br>
> >><br>
> >> So this algorithm is really conservative. I think the<br>
> >> solution might be removing this lowering or adding threshold<br>
> >> control for this lowering. Do you have any suggestions?<br>
> >><br>
> >> Thanks,<br>
> >> -Jiangning<br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >><br>
> >> ______________________________ _________________<br>
> >> llvm-commits mailing list<br>
> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/">http://lists.cs.uiuc.edu/</a> mailman/listinfo/llvm-commits<br>
> >><br>
> >><br>
> >><br>
> >> _______________________________________________<br>
> >> llvm-commits mailing list<br>
> >> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> >><br>
> ><br>
> > --<br>
> > Hal Finkel<br>
> > Assistant Computational Scientist<br>
> > Leadership Computing Facility<br>
> > Argonne National Laboratory<br>
> ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</p>