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'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?
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
