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

[Hal Finkel]

----- 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