* genattrab.c generate switch
@ 2016-01-13 0:53 Jesper Broge Jørgensen
2016-01-18 14:15 ` Bernd Schmidt
2016-02-01 19:20 ` Patrick Palka
0 siblings, 2 replies; 10+ messages in thread
From: Jesper Broge Jørgensen @ 2016-01-13 0:53 UTC (permalink / raw)
To: gcc-patches
Hello
genattrab.c can generate if statements that have very deep bracket
nesting causing clang to produce errors (when target=arm-none-eabi) as
explained at https://gcc.gnu.org/ml/gcc/2014-05/msg00032.html
At the above link it was suggested that genattrab.c generated a switch
statement instead. I have made a patch that does just that.
gcc/ChangeLog:
2016-01-13 Jesper Broge Jørgensen <jesperbroge@gmail.com>
* genattrtab.c (check_attr_set_switch): implemented the function
(write_attr_set): Check if expression can be written as a switch
diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 2caf8f6..b6de642 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -275,6 +275,8 @@ static bool attr_alt_subset_of_compl_p (rtx, rtx);
static void clear_struct_flag (rtx);
static void write_attr_valueq (FILE *, struct attr_desc *, const
char *);
static struct attr_value *find_most_used (struct attr_desc *);
+static int check_attr_set_switch (FILE *outf, rtx exp,
+ unsigned int attrs_cached, int write_cases, int
indent);
static void write_attr_set (FILE *, struct attr_desc *, int, rtx,
const char *, const char *, rtx,
int, int, unsigned int);
@@ -4113,6 +4115,102 @@ eliminate_known_true (rtx known_true, rtx exp,
int insn_code, int insn_index)
return exp;
}
+/* Check if exp contains a series of IOR conditions on the same attr_name.
+ If it does it can be turned into a switch statement and returns true.
+ If write_cases is true it will write the cases of the switch to
outf. */
+
+static int
+check_attr_set_switch (FILE *outf, rtx exp, unsigned int attrs_cached,
+ int write_cases, int indent)
+{
+ if (GET_CODE (exp) != IOR)
+ return 0;
+ if (GET_CODE (XEXP (exp, 0)) != EQ_ATTR)
+ return 0;
+
+ rtx next = exp;
+ int ior_depth = 0;
+ int is_first = 1;
+
+ const char *attr_name_cmp = XSTR (XEXP (exp, 0), 0);
+
+ while (1)
+ {
+ rtx op1 = XEXP (next, 0);
+ rtx op2 = XEXP (next, 1);
+
+ if (GET_CODE (op1) != EQ_ATTR)
+ return 0;
+
+ const char *attr_name = XSTR (op1, 0);
+ const char *cmp_val = XSTR (op1, 1);
+
+ /* pointer compare is enough. */
+ if (attr_name_cmp != attr_name)
+ return 0;
+
+ if (write_cases)
+ {
+ struct attr_desc *attr = find_attr (&attr_name, 0);
+ gcc_assert (attr);
+ if (is_first)
+ {
+ fprintf (outf, "(");
+ is_first = 0;
+ int i;
+ for (i = 0; i < cached_attr_count; i++)
+ if (attr->name == cached_attrs[i])
+ break;
+
+ if (i < cached_attr_count && (attrs_cached & (1U << i)) != 0)
+ fprintf (outf, "cached_%s", attr->name);
+ else if (i < cached_attr_count && (attrs_to_cache & (1U << i))
!= 0)
+ fprintf (outf, "(cached_%s = get_attr_%s (insn))", attr->name,
+ attr->name);
+ else
+ fprintf (outf, "get_attr_%s (insn)", attr->name);
+ fprintf (outf, ")\n");
+ write_indent (outf, indent);
+ fprintf (outf, "{\n");
+ }
+ write_indent (outf, indent);
+ fprintf (outf, "case ");
+ write_attr_valueq (outf, attr, cmp_val);
+ fprintf (outf, ":\n");
+ }
+
+ const int code = GET_CODE (op2);
+ if (code != IOR)
+ {
+ if (code == EQ_ATTR)
+ {
+ const char *attr_name = XSTR (op2, 0);
+ const char *cmp_val = XSTR (op2, 1);
+
+ if (attr_name == alternative_name)
+ return 0;
+
+ struct attr_desc *attr = find_attr (&attr_name, 0);
+ gcc_assert (attr);
+
+ if (attr->is_const)
+ return 0;
+ else if (write_cases)
+ {
+ write_indent (outf, indent);
+ fprintf (outf, "case ");
+ write_attr_valueq (outf, attr, cmp_val);
+ fprintf (outf, ":\n");
+ }
+ }
+ break;
+ }
+ next = op2;
+ ior_depth++;
+ }
+ return ior_depth > 2;
+}
+
/* Write out a series of tests and assignment statements to perform
tests and
sets of an attribute value. We are passed an indentation amount
and prefix
and suffix strings to write around each attribute value (e.g., "return"
@@ -4123,6 +4221,7 @@ write_attr_set (FILE *outf, struct attr_desc
*attr, int indent, rtx value,
const char *prefix, const char *suffix, rtx known_true,
int insn_code, int insn_index, unsigned int attrs_cached)
{
+ int n_switches = 0;
if (GET_CODE (value) == COND)
{
/* Assume the default value will be the default of the COND
unless we
@@ -4132,6 +4231,7 @@ write_attr_set (FILE *outf, struct attr_desc
*attr, int indent, rtx value,
rtx newexp;
int first_if = 1;
int i;
+ int is_switch = 0;
if (cached_attr_count)
{
@@ -4176,40 +4276,68 @@ write_attr_set (FILE *outf, struct attr_desc
*attr, int indent, rtx value,
if (inner_true == false_rtx)
continue;
+ is_switch = check_attr_set_switch (outf, testexp, attrs_cached, 0,
+ indent);
+
attrs_cached_inside = attrs_cached;
attrs_cached_after = attrs_cached;
write_indent (outf, indent);
- fprintf (outf, "%sif ", first_if ? "" : "else ");
- first_if = 0;
- write_test_expr (outf, testexp, attrs_cached,
- (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
- attrs_cached = attrs_cached_after;
- fprintf (outf, "\n");
- write_indent (outf, indent + 2);
- fprintf (outf, "{\n");
+ if (is_switch)
+ {
+ fprintf (outf, "switch ");
+ n_switches++;
+ first_if = 1;
+ check_attr_set_switch (outf, testexp, attrs_cached, 1, indent);
+ indent += 4;
+ }
+ else
+ {
+ fprintf (outf, "%sif ", first_if ? "" : "else ");
+ first_if = 0;
+ write_test_expr (outf, testexp, attrs_cached,
+ (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
+ attrs_cached = attrs_cached_after;
+ fprintf (outf, "\n");
+ }
+ if (! is_switch)
+ {
+ write_indent (outf, indent + 2);
+ fprintf (outf, "{\n");
+ }
+
write_attr_set (outf, attr, indent + 4,
XVECEXP (value, 0, i + 1), prefix, suffix,
inner_true, insn_code, insn_index,
attrs_cached_inside);
write_indent (outf, indent + 2);
- fprintf (outf, "}\n");
+ if (is_switch)
+ {
+ fprintf (outf, "break;\n");
+ write_indent (outf, indent);
+ fprintf (outf, "default:\n");
+ indent += 4;
+ }
+ else
+ fprintf (outf, "}\n");
our_known_true = newexp;
}
- if (! first_if)
+ if (! first_if && ! is_switch)
{
write_indent (outf, indent);
fprintf (outf, "else\n");
write_indent (outf, indent + 2);
fprintf (outf, "{\n");
}
+ else if (is_switch)
+ write_indent (outf, indent);
- write_attr_set (outf, attr, first_if ? indent : indent + 4,
default_val,
+ write_attr_set (outf, attr, (first_if || is_switch) ? indent :
indent + 4, default_val,
prefix, suffix, our_known_true, insn_code, insn_index,
attrs_cached);
- if (! first_if)
+ if (! first_if && ! is_switch)
{
write_indent (outf, indent + 2);
fprintf (outf, "}\n");
@@ -4222,6 +4350,12 @@ write_attr_set (FILE *outf, struct attr_desc
*attr, int indent, rtx value,
write_attr_value (outf, attr, value);
fprintf (outf, "%s\n", suffix);
}
+ while (n_switches--)
+ {
+ indent -= 2;
+ write_indent (outf, indent);
+ fprintf (outf, "}\n");
+ }
}
/* Write a series of case statements for every instruction in list IE.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-13 0:53 genattrab.c generate switch Jesper Broge Jørgensen
@ 2016-01-18 14:15 ` Bernd Schmidt
2016-01-18 14:22 ` Jakub Jelinek
2016-01-18 14:39 ` Jesper Broge Jørgensen
2016-02-01 19:20 ` Patrick Palka
1 sibling, 2 replies; 10+ messages in thread
From: Bernd Schmidt @ 2016-01-18 14:15 UTC (permalink / raw)
To: Jesper Broge Jørgensen, gcc-patches
On 01/13/2016 01:53 AM, Jesper Broge Jørgensen wrote:
> genattrab.c can generate if statements that have very deep bracket
> nesting causing clang to produce errors (when target=arm-none-eabi) as
> explained at https://gcc.gnu.org/ml/gcc/2014-05/msg00032.html
> At the above link it was suggested that genattrab.c generated a switch
> statement instead. I have made a patch that does just that.
Some preliminaries first - I don't see your name in existing ChangeLogs;
am I correct in assuming you've not gone through the copyright
assignment process?
Secondly, we're currently in a development phase where we only accept
bug fixes for gcc-6. You should resubmit/ping the patch once stage1
opens again.
> 2016-01-13 Jesper Broge Jørgensen <jesperbroge@gmail.com>
>
> * genattrtab.c (check_attr_set_switch): implemented the function
> (write_attr_set): Check if expression can be written as a switch
Please review our coding and documentation standards. ChangeLog entries
should be complete sentences (or sometimes brief short-hands: the first
one should just be "New function.")
> +static int check_attr_set_switch (FILE *outf, rtx exp,
> + unsigned int attrs_cached, int write_cases, int
> indent);
No reason to declare it if it is defined before its use.
> + while (1)
> + {
This and everything else here looks like it isn't following our
indentation rules.
Bernd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-18 14:15 ` Bernd Schmidt
@ 2016-01-18 14:22 ` Jakub Jelinek
2016-01-18 14:39 ` Jesper Broge Jørgensen
1 sibling, 0 replies; 10+ messages in thread
From: Jakub Jelinek @ 2016-01-18 14:22 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: Jesper Broge Jørgensen, gcc-patches
On Mon, Jan 18, 2016 at 03:15:08PM +0100, Bernd Schmidt wrote:
> Secondly, we're currently in a development phase where we only accept bug
> fixes for gcc-6. You should resubmit/ping the patch once stage1 opens again.
I think this is a bug fix, it is a workaround for a broken compiler that
some people use as system compiler to bootstrap gcc.
Jakub
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-18 14:15 ` Bernd Schmidt
2016-01-18 14:22 ` Jakub Jelinek
@ 2016-01-18 14:39 ` Jesper Broge Jørgensen
2016-01-18 17:39 ` Manuel López-Ibáñez
1 sibling, 1 reply; 10+ messages in thread
From: Jesper Broge Jørgensen @ 2016-01-18 14:39 UTC (permalink / raw)
To: Bernd Schmidt, gcc-patches
On 18/01/16 15:15, Bernd Schmidt wrote:
> On 01/13/2016 01:53 AM, Jesper Broge Jørgensen wrote:
>> genattrab.c can generate if statements that have very deep bracket
>> nesting causing clang to produce errors (when target=arm-none-eabi) as
>> explained at https://gcc.gnu.org/ml/gcc/2014-05/msg00032.html
>> At the above link it was suggested that genattrab.c generated a switch
>> statement instead. I have made a patch that does just that.
>
> Some preliminaries first - I don't see your name in existing
> ChangeLogs; am I correct in assuming you've not gone through the
> copyright assignment process?
>
> Secondly, we're currently in a development phase where we only accept
> bug fixes for gcc-6. You should resubmit/ping the patch once stage1
> opens again.
>
>> 2016-01-13 Jesper Broge Jørgensen <jesperbroge@gmail.com>
>>
>> * genattrtab.c (check_attr_set_switch): implemented the function
>> (write_attr_set): Check if expression can be written as a switch
>
> Please review our coding and documentation standards. ChangeLog
> entries should be complete sentences (or sometimes brief short-hands:
> the first one should just be "New function.")
>
>> +static int check_attr_set_switch (FILE *outf, rtx exp,
>> + unsigned int attrs_cached, int write_cases, int
>> indent);
>
> No reason to declare it if it is defined before its use.
>> + while (1)
>> + {
>
> This and everything else here looks like it isn't following our
> indentation rules.
>
>
> Bernd
No i have not gone through copyright assignment.
This is my first time trying to contribute to a GNU project so i have
tried following the "Contributing to GCC" at
https://gcc.gnu.org/contribute.html
There i followed the advice to run the patch through
contrib/check_GNU_style.sh and it came out clean. Maybe
contrib/check_GNU_style.sh does not check for indention rules and/or my
editor is set up wrongly so it looked to me like i was following the
coding standard.
I did not know you only accepted bug fixes though one could argue that
this fixes a (style)bug in generated code.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-18 14:39 ` Jesper Broge Jørgensen
@ 2016-01-18 17:39 ` Manuel López-Ibáñez
2016-01-18 22:44 ` Jesper Broge Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Manuel López-Ibáñez @ 2016-01-18 17:39 UTC (permalink / raw)
To: Jesper Broge Jørgensen, Bernd Schmidt, gcc-patches
On 18/01/16 14:39, Jesper Broge Jørgensen wrote:
> No i have not gone through copyright assignment.
> This is my first time trying to contribute to a GNU project so i have tried
> following the "Contributing to GCC"@
> https://gcc.gnu.org/contribute.html
> There i followed the advice to run the patch through contrib/check_GNU_style.sh
> and it came out clean. Maybe contrib/check_GNU_style.sh does not check for
> indention rules and/or my editor is set up wrongly so it looked to me like i
> was following the coding standard.
Hi Jesper,
Unfortunately, https://gcc.gnu.org/contribute.html is quite hard to follow and
outdated. I would suggest to start here:
https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps
From there, you'll get to https://gcc.gnu.org/wiki/FormattingCodeForGCC
If you know how to improve those pages, for example extending them to other
editors, I can give you write access.
Cheers,
Manuel.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-18 17:39 ` Manuel López-Ibáñez
@ 2016-01-18 22:44 ` Jesper Broge Jørgensen
2016-01-19 12:18 ` Bernd Schmidt
0 siblings, 1 reply; 10+ messages in thread
From: Jesper Broge Jørgensen @ 2016-01-18 22:44 UTC (permalink / raw)
To: Manuel López-Ibáñez, Bernd Schmidt, gcc-patches
On 18/01/16 18:39, Manuel López-Ibáñez wrote:
> On 18/01/16 14:39, Jesper Broge Jørgensen wrote:
>> No i have not gone through copyright assignment.
>> This is my first time trying to contribute to a GNU project so i have
>> tried
>> following the "Contributing to GCC"@
>> https://gcc.gnu.org/contribute.html
>> There i followed the advice to run the patch through
>> contrib/check_GNU_style.sh
>> and it came out clean. Maybe contrib/check_GNU_style.sh does not
>> check for
>> indention rules and/or my editor is set up wrongly so it looked to me
>> like i
>> was following the coding standard.
>
> Hi Jesper,
>
> Unfortunately, https://gcc.gnu.org/contribute.html is quite hard to
> follow and outdated. I would suggest to start here:
> https://gcc.gnu.org/wiki/GettingStarted#Basics:_Contributing_to_GCC_in_10_easy_steps
>
> From there, you'll get to https://gcc.gnu.org/wiki/FormattingCodeForGCC
>
> If you know how to improve those pages, for example extending them to
> other editors, I can give you write access.
>
> Cheers,
>
> Manuel.
>
Hi
I found a formatting tool called uncrustify that comes with a gnu style
config
https://github.com/bengardner/uncrustify/blob/master/etc/gnu-indent.cfg
that needed a few tweaks to format code that looked what is already in
gcc/genattrtab.c
The tweaks was:
indent_with_tabs = 2 // instead of 0
sp_func_def_paren = add // instead of remove
sp_func_proto_paren = add // instead of remove
sp_func_call_paren = add // instead of remove
So now the code should be correctly formatted.
Do i send in a new patch or just respond to the old one with the new
changes?
I have also followed instructions at
https://gcc.gnu.org/ml/gcc/2003-06/txt00010.txt to get copyright
assignment though i have not yet received a reply.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-18 22:44 ` Jesper Broge Jørgensen
@ 2016-01-19 12:18 ` Bernd Schmidt
2016-02-01 18:11 ` Jesper Broge Jørgensen
0 siblings, 1 reply; 10+ messages in thread
From: Bernd Schmidt @ 2016-01-19 12:18 UTC (permalink / raw)
To: Jesper Broge Jørgensen, Manuel López-Ibáñez,
gcc-patches
On 01/18/2016 11:44 PM, Jesper Broge Jørgensen wrote:
> I found a formatting tool called uncrustify that comes with a gnu style
> config
> https://github.com/bengardner/uncrustify/blob/master/etc/gnu-indent.cfg
> that needed a few tweaks to format code that looked what is already in
> gcc/genattrtab.c
>
> The tweaks was:
>
> indent_with_tabs = 2 // instead of 0
> sp_func_def_paren = add // instead of remove
> sp_func_proto_paren = add // instead of remove
> sp_func_call_paren = add // instead of remove
>
> So now the code should be correctly formatted.
Best to get that right when editing, though. emacs defaults to GNU style
and other editors can also be tweaked.
> Do i send in a new patch or just respond to the old one with the new
> changes?
Usually best to send updated patches (as text/plain attachment to avoid
word-wrapping and other whitespace damage).
> I have also followed instructions at
> https://gcc.gnu.org/ml/gcc/2003-06/txt00010.txt to get copyright
> assignment though i have not yet received a reply.
Ok, we'll have to wait for that.
Bernd
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-19 12:18 ` Bernd Schmidt
@ 2016-02-01 18:11 ` Jesper Broge Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Jesper Broge Jørgensen @ 2016-02-01 18:11 UTC (permalink / raw)
To: Bernd Schmidt, Manuel López-Ibáñez, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
On 19/01/16 13:18, Bernd Schmidt wrote:
> On 01/18/2016 11:44 PM, Jesper Broge Jørgensen wrote:
>> I found a formatting tool called uncrustify that comes with a gnu style
>> config
>> https://github.com/bengardner/uncrustify/blob/master/etc/gnu-indent.cfg
>> that needed a few tweaks to format code that looked what is already in
>> gcc/genattrtab.c
>>
>> The tweaks was:
>>
>> indent_with_tabs = 2 // instead of 0
>> sp_func_def_paren = add // instead of remove
>> sp_func_proto_paren = add // instead of remove
>> sp_func_call_paren = add // instead of remove
>>
>> So now the code should be correctly formatted.
>
> Best to get that right when editing, though. emacs defaults to GNU
> style and other editors can also be tweaked.
>
>> Do i send in a new patch or just respond to the old one with the new
>> changes?
>
> Usually best to send updated patches (as text/plain attachment to
> avoid word-wrapping and other whitespace damage).
>
>> I have also followed instructions at
>> https://gcc.gnu.org/ml/gcc/2003-06/txt00010.txt to get copyright
>> assignment though i have not yet received a reply.
>
> Ok, we'll have to wait for that.
>
>
> Bernd
>
I have finally received the copyright assignment it is currently on its
way over the atlantic via snail mail.
In the meantime i have attached the the refomatted patch here hoping it
could be reviewed while we wait.
[-- Attachment #2: gcc_patch_reformatted.patch --]
[-- Type: text/plain, Size: 6503 bytes --]
gcc/ChangeLog:
2016-01-19 Jesper Broge Jørgensen <jesperbroge@gmail.com>
* genattrtab.c (check_attr_set_switch): New function
(write_attr_set): Write a switch instead of if condition if possible
diff --git a/gcc/genattrtab.c b/gcc/genattrtab.c
index 2caf8f6..8e7f9e6 100644
--- a/gcc/genattrtab.c
+++ b/gcc/genattrtab.c
@@ -4113,6 +4113,103 @@ eliminate_known_true (rtx known_true, rtx exp, int insn_code, int insn_index)
return exp;
}
+/* Check if exp contains a series of IOR conditions on the same attr_name.
+ If it does it can be turned into a switch statement and returns true.
+ If write_cases is true it will write the cases of the switch to outf. */
+
+static int
+check_attr_set_switch (FILE *outf, rtx exp, unsigned int attrs_cached,
+ int write_cases, int indent)
+{
+ if (GET_CODE (exp) != IOR)
+ return 0;
+ if (GET_CODE (XEXP (exp, 0)) != EQ_ATTR)
+ return 0;
+
+ rtx next = exp;
+ int ior_depth = 0;
+ int is_first = 1;
+
+ const char *attr_name_cmp = XSTR (XEXP (exp, 0), 0);
+
+ while (1)
+ {
+ rtx op1 = XEXP (next, 0);
+ rtx op2 = XEXP (next, 1);
+
+ if (GET_CODE (op1) != EQ_ATTR)
+ return 0;
+
+ const char *attr_name = XSTR (op1, 0);
+ const char *cmp_val = XSTR (op1, 1);
+
+ /* pointer compare is enough. */
+ if (attr_name_cmp != attr_name)
+ return 0;
+
+ if (write_cases)
+ {
+ struct attr_desc *attr = find_attr (&attr_name, 0);
+ gcc_assert (attr);
+ if (is_first)
+ {
+ fprintf (outf, "(");
+ is_first = 0;
+ int i;
+ for (i = 0; i < cached_attr_count; i++)
+ if (attr->name == cached_attrs[i])
+ break;
+
+ if (i < cached_attr_count && (attrs_cached & (1U << i)) != 0)
+ fprintf (outf, "cached_%s", attr->name);
+ else if (i < cached_attr_count &&
+ (attrs_to_cache & (1U << i)) != 0)
+ fprintf (outf, "(cached_%s = get_attr_%s (insn))", attr->name,
+ attr->name);
+ else
+ fprintf (outf, "get_attr_%s (insn)", attr->name);
+ fprintf (outf, ")\n");
+ write_indent (outf, indent);
+ fprintf (outf, "{\n");
+ }
+ write_indent (outf, indent);
+ fprintf (outf, "case ");
+ write_attr_valueq (outf, attr, cmp_val);
+ fprintf (outf, ":\n");
+ }
+
+ const int code = GET_CODE (op2);
+ if (code != IOR)
+ {
+ if (code == EQ_ATTR)
+ {
+ const char *attr_name = XSTR (op2, 0);
+ const char *cmp_val = XSTR (op2, 1);
+
+ if (attr_name == alternative_name)
+ return 0;
+
+ struct attr_desc *attr = find_attr (&attr_name, 0);
+ gcc_assert (attr);
+
+ if (attr->is_const)
+ return 0;
+ else if (write_cases)
+ {
+ write_indent (outf, indent);
+ fprintf (outf, "case ");
+ write_attr_valueq (outf, attr, cmp_val);
+ fprintf (outf, ":\n");
+ }
+ }
+ break;
+ }
+ next = op2;
+ ior_depth++;
+ }
+ return ior_depth > 2;
+}
+
/* Write out a series of tests and assignment statements to perform tests and
sets of an attribute value. We are passed an indentation amount and prefix
and suffix strings to write around each attribute value (e.g., "return"
@@ -4123,6 +4220,7 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
const char *prefix, const char *suffix, rtx known_true,
int insn_code, int insn_index, unsigned int attrs_cached)
{
+ int n_switches = 0;
if (GET_CODE (value) == COND)
{
/* Assume the default value will be the default of the COND unless we
@@ -4132,6 +4230,7 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
rtx newexp;
int first_if = 1;
int i;
+ int is_switch = 0;
if (cached_attr_count)
{
@@ -4176,40 +4275,68 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
if (inner_true == false_rtx)
continue;
+ is_switch = check_attr_set_switch (outf, testexp, attrs_cached, 0,
+ indent);
+
attrs_cached_inside = attrs_cached;
attrs_cached_after = attrs_cached;
write_indent (outf, indent);
- fprintf (outf, "%sif ", first_if ? "" : "else ");
- first_if = 0;
- write_test_expr (outf, testexp, attrs_cached,
- (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
- attrs_cached = attrs_cached_after;
- fprintf (outf, "\n");
- write_indent (outf, indent + 2);
- fprintf (outf, "{\n");
+ if (is_switch)
+ {
+ fprintf (outf, "switch ");
+ n_switches++;
+ first_if = 1;
+ check_attr_set_switch (outf, testexp, attrs_cached, 1, indent);
+ indent += 4;
+ }
+ else
+ {
+ fprintf (outf, "%sif ", first_if ? "" : "else ");
+ first_if = 0;
+ write_test_expr (outf, testexp, attrs_cached,
+ (FLG_AFTER | FLG_INSIDE | FLG_OUTSIDE_AND));
+ attrs_cached = attrs_cached_after;
+ fprintf (outf, "\n");
+ }
+ if (!is_switch)
+ {
+ write_indent (outf, indent + 2);
+ fprintf (outf, "{\n");
+ }
+
write_attr_set (outf, attr, indent + 4,
XVECEXP (value, 0, i + 1), prefix, suffix,
inner_true, insn_code, insn_index,
attrs_cached_inside);
write_indent (outf, indent + 2);
- fprintf (outf, "}\n");
+ if (is_switch)
+ {
+ fprintf (outf, "break;\n");
+ write_indent (outf, indent);
+ fprintf (outf, "default:\n");
+ indent += 4;
+ }
+ else
+ fprintf (outf, "}\n");
our_known_true = newexp;
}
- if (! first_if)
+ if (! first_if && ! is_switch)
{
write_indent (outf, indent);
fprintf (outf, "else\n");
write_indent (outf, indent + 2);
fprintf (outf, "{\n");
}
+ else if (is_switch)
+ write_indent (outf, indent);
- write_attr_set (outf, attr, first_if ? indent : indent + 4, default_val,
+ write_attr_set (outf, attr, (first_if || is_switch) ? indent : indent + 4, default_val,
prefix, suffix, our_known_true, insn_code, insn_index,
attrs_cached);
- if (! first_if)
+ if (! first_if && ! is_switch)
{
write_indent (outf, indent + 2);
fprintf (outf, "}\n");
@@ -4222,6 +4349,12 @@ write_attr_set (FILE *outf, struct attr_desc *attr, int indent, rtx value,
write_attr_value (outf, attr, value);
fprintf (outf, "%s\n", suffix);
}
+ while (n_switches--)
+ {
+ indent -= 2;
+ write_indent (outf, indent);
+ fprintf (outf, "}\n");
+ }
}
/* Write a series of case statements for every instruction in list IE.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-01-13 0:53 genattrab.c generate switch Jesper Broge Jørgensen
2016-01-18 14:15 ` Bernd Schmidt
@ 2016-02-01 19:20 ` Patrick Palka
2016-02-01 20:05 ` Jesper Broge Jørgensen
1 sibling, 1 reply; 10+ messages in thread
From: Patrick Palka @ 2016-02-01 19:20 UTC (permalink / raw)
To: Jesper Broge Jørgensen; +Cc: GCC Patches
On Tue, Jan 12, 2016 at 7:53 PM, Jesper Broge Jørgensen
<jesperbroge@gmail.com> wrote:
> Hello
>
> genattrab.c can generate if statements that have very deep bracket nesting
> causing clang to produce errors (when target=arm-none-eabi) as explained at
> https://gcc.gnu.org/ml/gcc/2014-05/msg00032.html
> At the above link it was suggested that genattrab.c generated a switch
> statement instead. I have made a patch that does just that.=
Have you considered first implementing the other suggestion in that
thread -- to avoid emitting the redundant parentheses in a consecutive
chain ||s and &&s? That kind of fix would be simpler and would fix the
compilation issue all the same, right?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: genattrab.c generate switch
2016-02-01 19:20 ` Patrick Palka
@ 2016-02-01 20:05 ` Jesper Broge Jørgensen
0 siblings, 0 replies; 10+ messages in thread
From: Jesper Broge Jørgensen @ 2016-02-01 20:05 UTC (permalink / raw)
To: Patrick Palka; +Cc: GCC Patches
On 01/02/16 20:19, Patrick Palka wrote:
> On Tue, Jan 12, 2016 at 7:53 PM, Jesper Broge Jørgensen
> <jesperbroge@gmail.com> wrote:
>> Hello
>>
>> genattrab.c can generate if statements that have very deep bracket nesting
>> causing clang to produce errors (when target=arm-none-eabi) as explained at
>> https://gcc.gnu.org/ml/gcc/2014-05/msg00032.html
>> At the above link it was suggested that genattrab.c generated a switch
>> statement instead. I have made a patch that does just that.=
> Have you considered first implementing the other suggestion in that
> thread -- to avoid emitting the redundant parentheses in a consecutive
> chain ||s and &&s? That kind of fix would be simpler and would fix the
> compilation issue all the same, right?
I think it makes more sense to have the special case logic as close to
where you know that you have to handle it eg. in write_attr_set()
instead of in the more general purpos write_test_expr() (which is the
where the parentheses is inserted). Also creating a different flow
control structure (writing a switch instead of an if statement) makes it
clear that we are handling an edge case. Also most of the logic needed
is in the new function check_attr_set_switch() that is used to check if
the expression is indeed a chain of || operators which would have to be
done either way, so not much simplification would be gained.
write_test_expr() also makes the promise (in form of an inline comment)
that parentheses are inserted to avoid worrying about operator
precedence, having a special case that does not insert parentheses would
break that promise.
Lastly i would also guess that a switch statement could be better
optimized by the compiler but one would have to inspect the assembly
output which i have not done so that is guess-work.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-02-01 20:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-13 0:53 genattrab.c generate switch Jesper Broge Jørgensen
2016-01-18 14:15 ` Bernd Schmidt
2016-01-18 14:22 ` Jakub Jelinek
2016-01-18 14:39 ` Jesper Broge Jørgensen
2016-01-18 17:39 ` Manuel López-Ibáñez
2016-01-18 22:44 ` Jesper Broge Jørgensen
2016-01-19 12:18 ` Bernd Schmidt
2016-02-01 18:11 ` Jesper Broge Jørgensen
2016-02-01 19:20 ` Patrick Palka
2016-02-01 20:05 ` Jesper Broge Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).