This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add if-chain to switch conversion pass.
- From: Martin Liška <mliska at suse dot cz>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org, kazu at gcc dot gnu dot org, Jan Hubicka <hubicka at ucw dot cz>
- Date: Wed, 13 Nov 2019 12:22:31 +0100
- Subject: Re: [PATCH] Add if-chain to switch conversion pass.
- References: <2c3db526-cac6-4eeb-4afb-12024f8d5af2@suse.cz> <20191104144851.GJ4650@tucnak>
On 11/4/19 3:48 PM, Jakub Jelinek 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.
Thank you for it.
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?
No, it's currently not supported. I can consider adding that as a follow up.
2) for the reassoc-*.c testcases, do you get identical or better code
with the patch?
The code is the following:
Before:
Optimizing range tests a_5(D) -[10, 10] and -[26, 26]
into (a_5(D) & -17) != 10
<bb 2> [local count: 1073741823]:
_11 = a_5(D) & -17;
_12 = _11 == 10;
_1 = a_5(D) == 10;
_2 = a_5(D) == 12;
_10 = a_5(D) == 26;
_9 = _2 | _12;
if (_9 != 0)
goto <bb 4>; [56.44%]
else
goto <bb 3>; [43.56%]
After:
<bb 2> [local count: 1073741824]:
switch (a_2(D)) <default: <L5> [50.00%], case 10: <L6> [50.00%], case 12: <L6> [50.00%], case 26: <L6> [50.00%]>
As seen, reassoc can generate a different range check and then it's about cmp1 | cmp2 | ..
I bet the explicit gswitch is quite equal representation.
3) shouldn't it be gimple-if-to-switch.c instead?
Yes, I renamed that.
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)?
I've just measures that and it's about:
VM SIZE FILE SIZE
++++++++++++++ GROWING ++++++++++++++
+0.3% +24.8Ki .rodata +24.8Ki +0.3%
[ = ] 0 .debug_loc +17.4Ki +0.0%
[ = ] 0 .debug_line +8.77Ki +0.0%
+0.0% +2.52Ki .text +2.52Ki +0.0%
+0.1% +1.66Ki .eh_frame +1.66Ki +0.1%
[ = ] 0 .symtab +216 +0.0%
[ = ] 0 .debug_abbrev +32 +0.0%
+0.0% +16 .eh_frame_hdr +16 +0.0%
+0.1% +29.0Ki TOTAL +44.7Ki +0.0%
Martin
+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