[PATCH] Add threshold for lowering lattice value 'overdefined' in LVI

[Hal Finkel]

----- Original Message -----
> From: "Jiangning Liu" <liujiangning1 at gmail.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D5322+public+26bcfcc2c04b0b02 at reviews.llvm.org, mcrosier at codeaurora.org, joerg at netbsd.org,
> "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Tuesday, September 16, 2014 12:07:21 AM
> Subject: Re: [PATCH] Add threshold for lowering lattice value 'overdefined' in LVI
> 
> 
> Hi Hal,
> 
> 
> I don't understand why putting the count inside LVILatticeVal can be
> helpful to the dilemma you mentioned? Can you explain more about
> this? And why it can be more local that way?

Is the problem not that, in this worst case, we're doing too many expensive visits to some (LVILatticeVal, BB) pairs? With a global threshold for all (LVILatticeVal, BB) visits, we can do more optimization in some parts of the function than in others (because we'll visit some parts of the function before other parts). If we keep the count per value, then we limit the analysis done per value, but that is independent of where in the function it is.

> 
> 
> I think at present we have an unique ValueCacheEntryTy for each value
> on a specific block. If we put a count inside LVILatticeVal, what is
> the semantic of this count?

My thought was that it would be the same as the count you originally proposed, but per value. So we'd count the number of times the value had been marked as over-defined. And once the limit is reached, we'd give up.

 -Hal

> 
> 
> Thanks,
> -Jiangning
> 
> 
> 
> 
> 2014-09-16 2:51 GMT+08:00 Hal Finkel < hfinkel at anl.gov > :
> 
> 
> 
> 
> ----- Original Message -----
> > From: "Jiangning Liu" < liujiangning1 at gmail.com >
> > To: liujiangning1 at gmail.com , hfinkel at anl.gov
> > Cc: mcrosier at codeaurora.org , joerg at NetBSD.org,
> > llvm-commits at cs.uiuc.edu
> > Sent: Sunday, September 14, 2014 10:18:49 PM
> > Subject: Re: [PATCH] Add threshold for lowering lattice value
> > 'overdefined' in LVI
> > 
> > Hi Hal, Jeorg and Chad,
> > 
> > Thanks for your feedback!
> > 
> > -Jiangning
> > 
> > ================
> > Comment at: lib/Analysis/LazyValueInfo.cpp:41
> > @@ -39,1 +40,3 @@
> > 
> > +// This threshold is purticularly tuned with some special cases
> > exposing
> > +// regression of compile time and memory consumption.
> > ----------------
> > joerg wrote:
> > > Typo. Maybe with the changes suggsted by Hal use just:
> > > 
> > > // Experimentally derived threshold for additional lowering
> > > lattice
> > > values per block
> > OK.
> > 
> > ================
> > Comment at: lib/Analysis/LazyValueInfo.cpp:360
> > @@ -351,1 +359,3 @@
> > + /// lowered.
> > + unsigned LoweringOverdefinedTimes;
> > 
> > ----------------
> > hfinkel wrote:
> > > Could you make this a DenseMap<BasicBlock *, unsigned>, and keep
> > > a
> > > count per block?
> > OK. But for this test case, the algorithm just encounters too many
> > basic blocks rather than too many times for a single basic block.
> > Anyway, to avoid confusion, following your comments, I added
> > another
> > threshold to check the number of basic blocks, so now we have two
> > thresholds.
> 
> Alright. Here's what I'm trying to avoid: Given a large function,
> optimizing code near the beginning of the function better than code
> at the end of the function. Maybe we should put the count inside of
> LVILatticeVal itself? That seems like it should be more local.
> 
> Thanks again,
> Hal
> 
> 
> 
> > 
> > 
> > 
> > ================
> > Comment at: lib/Analysis/LazyValueInfo.cpp:556
> > @@ -542,1 +555,3 @@
> > + // complexity. Therefore, we introduce OverdefinedThreshold to
> > control
> > + // the maximum times of further lowering lattice value
> > 'overdefined'.
> > 
> > ----------------
> > joerg wrote:
> > > Maybe just:
> > > 
> > > // Limit the number of additional lowering lattice value to avoid
> > > unjustified memory grows.
> > OK.
> > 
> > ================
> > Comment at: lib/Analysis/LazyValueInfo.cpp:574
> > @@ -556,2 +573,3 @@
> > BBLV.markOverdefined();
> > + LoweringOverdefinedTimes++;
> > 
> > ----------------
> > mcrosier wrote:
> > > Please use a preincrement.
> > > 
> > > http://llvm.org/releases/2.7/docs/CodingStandards.html#micro_preincrement
> > OK.
> > 
> > http://reviews.llvm.org/D5322
> > 
> > 
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory