Re: [PATCH] Add if-chain to switch conversion pass.

[Bernhard Reutner-Fischer <rep dot dot dot nop at gmail dot com>]

On Tue, 5 Nov 2019 13:38:27 +0100
Richard Biener <richard.guenther@gmail.com> wrote:

> On Mon, Nov 4, 2019 at 3:49 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Mon, Nov 04, 2019 at 03:23:20PM +0100, Martin Liška wrote:  
> > > The patch adds a new pass that identifies a series of if-elseif
> > > statements and transform then into a GIMPLE switch (if possible).
> > > The pass runs right after tree-ssa pass and I decided to implement
> > > matching of various forms that are introduced by folder (fold_range_test):  
> >
> > Not a review, just a few questions:  
> 
> Likewise - please do not name switches -ftree-*, 'tree' doens't add anything
> but confusion to users.  Thus use -fif-to-switch or -fconvert-if-to-switch
> 
> +The transformation can help to produce a faster code for
> +the switch statement.
> 
> produce faster code.
> 
> Doesn't it also produce smaller code eventually?
> 
> Please to not put code transform passes into build_ssa_passes (why did
> you choose this place)?  The pass should go into pass_all_early_optimizations
> instead, and I'm quite sure you want to run _after_ CSE.  I'd even say
> that the pass should run as part of switch-conversion, so we build
> a representation of a switch internally and then code-generate the optimal
> form directly.  For now just put the pass before switch-conversion.

Also why do you punt on duplicate conditions like in

> +++ b/gcc/testsuite/gcc.dg/tree-ssa/if-to-switch-4.c

> +int main(int argc, char **argv)
> +{
> +  if (argc == 1)
> +  else if (argc == 2)
> +  else if (argc == 3)
> +  else if (argc == 4)
> +  else if (argc == 1)
> +    {

This block is dead, isn't it. Why don't you just discard it but punt?

> +      global = 2;
> +    }
> +  else
> +    global -= 123;

thanks,
> 
> There are functions without comments in the patch and you copied
> from DSE which shows in confusing comments left over from the original.
> 
> +  mark_virtual_operands_for_renaming (cfun);
> 
> if you did nothing renaming all vops is expensive.
> 
> I'm missing an overall comment - you are using a dominator walk
> but do nothing in the after hook which means you are not really
> gathering any data?  You're also setting visited bits on BBs which
> means you are visiting alternate BBs during the DOM walk.
> 
> > 1) what does it do if __builtin_expect* has been used, does it preserve
> >    the probabilities and if in the end decides to expand as ifs, are those
> >    probabilities retained through it?
> > 2) for the reassoc-*.c testcases, do you get identical or better code
> >    with the patch?
> > 3) shouldn't it be gimple-if-to-switch.c instead?
> > 4) what code size effect does the patch have say on cc1plus (if you don't
> >    count the code changes of the patch itself, i.e. revert the patch in the
> >    stage3 and rebuild just the stage3)?
> >  
> > > +struct case_range
> > > +{
> > > +  /* Default constructor.  */
> > > +  case_range ():
> > > +    m_min (NULL_TREE), m_max (NULL_TREE)  
> >
> > I admit I'm never sure about coding conventions for C++,
> > but shouldn't there be a space before :, or even better :
> > be on the next line before m_min ?
> >
> >         Jakub
> >