Message ID | 56959FF8.9070908@gmail.com |
---|---|
State | New |
Headers | show |
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
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
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.
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.
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.
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
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?
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.
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,