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

[Pete Cooper]

> On Nov 20, 2014, at 11:10 AM, Hans Wennborg <hans at chromium.org> wrote:
> 
> Sorry for coming late to this thread.
> 
> On Fri, Oct 10, 2014 at 6:31 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>> ----- Original Message -----
>>> From: "Jiangning Liu" <liujiangning1 at gmail.com>
>>> To: "Hal Finkel" <hfinkel at anl.gov>
>>> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>, "Nick Lewycky" <nicholas at mxc.ca>, "Daniel Berlin"
>>> <dberlin at dberlin.org>
>>> Sent: Friday, October 10, 2014 3:29:25 AM
>>> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,
>>> If you have any other empirical evidence, could you please share it?
>> 
>> No, but I think it became clear that there were serious algorithmic concerns, justified by the fact that we needed a cut-off in the first place. The fact remains that, initially, we did not think that this commit would change the asymptotic complexity of the algorithm. I remember thinking that it would only cause additional processing to happen for blocks we had already decided to visit, not cause any additional visits, and so I expected, at most, a constant-factor increase. This turned out to be wrong, and I think that Danny and Nick have done a good job explaining the error in my reasoning.
>> 
>> So at this point we're having a design discussion about a proper solution, and that's great. Nevertheless, the reasoning behind approving the current implementation was that it did not change the complexity, which was wrong, and based on Danny's and Nick's notes, I'm also not convinced that the cut-offs we introduced were proper (meaning minimal). As a matter of general policy, we don't keep commits in tree for which there are serious algorithmic concerns, and so reverting was appropriate.
>> 
>> Please do continue to work on this!
> 
> Is there any update on this patch? When it got reverted, we saw a 110
> k binary size increase in Chromium.
I came across this independently a few days ago.  My example was similar.  I had

if (x == 0) {
  if (…) {  … } else { … }
  ++x
}

The problem here is that as we walk from ++x up, we visit the BBs on the inner if statement.  Their predecessors haven’t been processed yet so we return out from solveBlockValueNonLocal which leaves them in the ‘overdefined’ state.  We eventually get to ‘x == 0’ and start the walk back down the CFG.  But on the walk back down we do the following check and immediately bail.  So intermediate BBs get set to overdefined because of unvisited predecessors and never leave that state.

if (!BBLV.isUndefined()) {
    DEBUG(dbgs() << "  reuse BB '" << BB->getName() << "' val=" << BBLV <<'\n');
    
    // Since we're reusing a cached value here, we don't need to update the 
    // OverDefinedCahce.  The cache will have been properly updated 
    // whenever the cached value was inserted.
    ODCacheUpdater.markResult(false);
    return true;
  }

So, here’s the patch I have to fix the problem.  I believe it has no algorithmic complexity issues as we do the same walk up the CFG from before, but as we walk back down we will retry calling solveBlockValueNonLocal now that all predecessors have a state.  I don’t have an IR test case this minute, but I can get one tonight if needed.

@@ -672,8 +672,16 @@ bool LazyValueInfoCache::solveBlockValueNonLocal(LVILatticeVal &BBLV,
       return true;
     }
   }
-  if (EdgesMissing)
+  if (EdgesMissing) {
+    // If we haven't yet processed all the predecessors, then put this
+    // value back to undefined so that we'll revisit it again.  Otherwise it
+    // will be 'overdefined' once we return from here and we'll think that
+    // this is its final state, while in fact we don't know the state of this
+    // value in this BB yet.
+    LVILatticeVal Result;
+    BBLV = Result;
     return false;
+  }

Thanks,
Pete
> 
> - Hans
> 
> _______________________________________________
> 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/20141120/da21597a/attachment.html>