[PATCH] Improving LowerSwitch behavior for contiguous ranges

[Jim Grosbach]

Hi Marcello,

This is awesome. I’ve been wanting improvements like this for a long time now, but have never gotten around to doing it myself. My procrastination^Wpatience is paying off!

When running on the LLVM test suite (including externals, preferably), how often does this optimization fire?

A few detail comments below. Nothing major.

General style nitpick: make sure comments are full sentences and end in a period.


> +// LowerBound and UpperBound are used to keep track of the bounds for Val       
> +// that have being already checked by a block emitted by one of the previous    
> +// calls to switchConvert in the call stack.   

s/have being already/have already been/

> +    // Check if the Case Range is perfectly squeezed in between
> +    // already checked Upper and Lower bounds. If it is then we can avoid
> +    // emitting the code that checks if the value actually falls in the range
> +    // because the bounds already tell us so
> +    if (LowerBound != nullptr && UpperBound != nullptr &&
> +        Begin->Low == LowerBound && Begin->High == UpperBound) {
> +      return Begin->BB;
> +    }

Can Begin->Low or Begin->High ever be null? If not, the explicit nullptr checks can be removed as they’re implicit. If they can be null, that seems a bit odd for the algorithm.

> +  // NewLowerBound here should never be the integer value minimal.

I don’t quite follow this comment.

> +  // Optimize the case where Default is an unreachable block                    
> +  if (DefaultIsUnreachable) {                                                   
> +    CaseItr LastCase = Cases.begin() + Cases.size() - 1;                        
> +    UpperBound = cast<ConstantInt>(LastCase->High);                             
> +    LowerBound = cast<ConstantInt>(Cases.begin()->Low);                         
> +  }


While accurate, the comment is a bit terse. It would be helpful to explain how we’re optimizing it, not just that we are. I.e., explain why the following code results in an optimization.


> On Jun 11, 2014, at 4:22 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
> 
> I'm updating the patch to also remove the default basic block if it ends up being dead after switch lowering (no predecessors)
> 
> Marcello
> 
> 2014-06-11 20:34 GMT+01:00 Marcello Maggioni <hayarms at gmail.com>:
> Joerg,
> 
> I addressed your suggestion in this patch.
> It was quite easy to add and can be useful in general, so thanks!
> 
> I also added a test that tests this kind of optimization being applied.
> 
> Marcello
> 
> 
> 2014-06-11 18:37 GMT+01:00 Joerg Sonnenberger <joerg at britannica.bec.de>:
> 
> On Wed, Jun 11, 2014 at 05:28:05PM +0100, Marcello Maggioni wrote:
> > Yeah, it is suboptimal, it doesn't take into consideration the fact that
> > the default is unreachable.
> >
> > I'll a look at it later to see if it is easy to also take into
> > consideration this case and add it to the patch + test for the condition.
> 
> Thanks. Please don't take this as hold up for pushing the original
> patch, it can just as well be a follow-up commit.
> 
> Joerg
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> <optimized_lower_switch_v5.patch>_______________________________________________
> 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/20140612/b18ab7af/attachment.html>