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

[Richard Biener <richard.guenther@gmail.com>]

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.

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
>