On Tue, 5 Nov 2019 13:38:27 +0100
Richard Biener <richard.guent...@gmail.com> wrote:
> On Mon, Nov 4, 2019 at 3:49 PM Jakub Jelinek <ja...@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
> >
