On Thu, Nov 14, 2019 at 10:39 AM Martin Liška <mli...@suse.cz> wrote:
>
> On 11/5/19 1:38 PM, Richard Biener 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:
>
> Hello.
>
> >
> > 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
>
> Agree with you, I selected the later option.
>
> >
> > +The transformation can help to produce a faster code for
> > +the switch statement.
> >
> > produce faster code.
>
> Fixed.
>
> >
> > Doesn't it also produce smaller code eventually?
>
> In some situation yes, but generally it leads to more jump tables
> (which are bigger when expanded).
>
> >
> > Please to not put code transform passes into build_ssa_passes (why did
> > you choose this place)?
>
> Well, that was my initial pass selection as I wanted to have a GIMPLE
> code close to what FEs produce.
>
> > 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.
>
> But yes, the suggested place is much better place and we can benefit from
> VRP (that will kill dead conditions in a if-else chain)
>
> >
> > 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.
>
> This one is needed for situations like:
>
> fn1 ()
> {
> <unnamed type> a.0_1;
>
> <bb 2> :
> # VUSE <.MEM_5(D)>
> a.0_1 = a;
> if (a.0_1 == 0)
> goto <bb 6>; [INV]
> else
> goto <bb 3>; [INV]
>
> <bb 3> :
> if (a.0_1 == 1)
> goto <bb 6>; [INV]
> else
> goto <bb 4>; [INV]
>
> <bb 4> :
> if (a.0_1 == 2)
> goto <bb 5>; [INV]
> else
> goto <bb 6>; [INV]
>
> <bb 5> :
> # .MEM_6 = VDEF <.MEM_5(D)>
> fn2 ();
>
> <bb 6> :
> # .MEM_4 = PHI <.MEM_5(D)(2), .MEM_5(D)(3), .MEM_5(D)(4), .MEM_6(5)>
> # VUSE <.MEM_4>
> return;
>
> }
>
> where without the call, I end up with:
>
> <bb 2> :
> # VUSE <.MEM_5(D)>
> a.0_1 = a;
> switch (a.0_1) <default: <L8> [INV], case 0: <L8> [INV], case 1: <L8>
> [INV], case 2: <L9> [INV]>
>
> <bb 3> :
> <L9>:
> # .MEM_6 = VDEF <.MEM_5(D)>
> fn2 ();
>
> <bb 4> :
> # .MEM_4 = PHI <.MEM_6(3), (2)>
I meant you are doing it unconditionally even if you didn't transform
any if-then-else chain.
>
> >
> > 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.
>
> You are right, I'm a bit cheating with the DOM walk as I also mark visited
> BBs.
> What I want is for each if-else condition, I want to visit the first IF in
> such
> chain. That's why I decided to iterate in DOMINATOR order.
> Can I do it simpler?
Put that in a comment, do away with domwalk and instead start from
ENTRY_BLOCK, using a worklist seeded by {first,next}_dom_son ()
and avoid putting visited blocks on that worklist.
Btw, what about if-chains nested in another if-chain? Don't you want to
transform "inner" chains first or does it really not matter (you're adjusting
the CFG, doing that inside the domwalk is fishy since that also uses
pre-computed RPO order; the simple dom-son walking should work
but you of course might miss some blocks depending on how you set up
things).
Richard.
> Thanks,
> Martin
>
> >
> >> 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
> >>
>
