* [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]
@ 2020-07-23 8:44 Hu Jiangping
2020-07-24 7:56 ` Hu, Jiangping
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Hu Jiangping @ 2020-07-23 8:44 UTC (permalink / raw)
To: gcc-patches
Thanks, Richard!
I think your suggestion is very good, so I made a new patch.
v2: at a high level handles -falign-foo=0 like -falign-foo
v1: at the target level overides the -falign-foo=0 option values
Obviously, v2 is better than v1. In addition, anthor option
to reject 0 that discussed in the email and PR96247
is not as good as the current patch either, I think.
I tested this patch on x86_64, it works well. OK for trunk?
Regards!
Hujp
---
gcc/opts.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/gcc/opts.c b/gcc/opts.c
index 499eb900643..ed6102cd606 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -2786,18 +2786,38 @@ common_handle_option (struct gcc_options *opts,
case OPT_falign_loops_:
check_alignment_argument (loc, arg, "loops");
+ // fix PR96247
+ if (0 == atoi(arg)) {
+ opts->x_flag_align_loops = true;
+ opts->x_str_align_loops = NULL;
+ }
break;
case OPT_falign_jumps_:
check_alignment_argument (loc, arg, "jumps");
+ // fix PR96247
+ if (0 == atoi(arg)) {
+ opts->x_flag_align_jumps = true;
+ opts->x_str_align_jumps = NULL;
+ }
break;
case OPT_falign_labels_:
check_alignment_argument (loc, arg, "labels");
+ // fix PR96247
+ if (0 == atoi(arg)) {
+ opts->x_flag_align_labels = true;
+ opts->x_str_align_labels = NULL;
+ }
break;
case OPT_falign_functions_:
check_alignment_argument (loc, arg, "functions");
+ // fix PR96247
+ if (0 == atoi(arg)) {
+ opts->x_flag_align_functions = true;
+ opts->x_str_align_functions = NULL;
+ }
break;
case OPT_ftabstop_:
--
2.17.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]
2020-07-23 8:44 [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247] Hu Jiangping
@ 2020-07-24 7:56 ` Hu, Jiangping
2020-07-24 13:01 ` Segher Boessenkool
2020-07-24 15:50 ` Richard Sandiford
2 siblings, 0 replies; 5+ messages in thread
From: Hu, Jiangping @ 2020-07-24 7:56 UTC (permalink / raw)
To: gcc-patches
Add CC to Richard.
> Thanks, Richard!
>
> I think your suggestion is very good, so I made a new patch.
>
> v2: at a high level handles -falign-foo=0 like -falign-foo
> v1: at the target level overides the -falign-foo=0 option values
>
> Obviously, v2 is better than v1. In addition, anthor option
> to reject 0 that discussed in the email and PR96247
> is not as good as the current patch either, I think.
>
> I tested this patch on x86_64, it works well. OK for trunk?
>
> Regards!
> Hujp
>
> ---
> gcc/opts.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/gcc/opts.c b/gcc/opts.c
> index 499eb900643..ed6102cd606 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -2786,18 +2786,38 @@ common_handle_option (struct gcc_options
> *opts,
>
> case OPT_falign_loops_:
> check_alignment_argument (loc, arg, "loops");
> + // fix PR96247
> + if (0 == atoi(arg)) {
> + opts->x_flag_align_loops = true;
> + opts->x_str_align_loops = NULL;
> + }
> break;
>
> case OPT_falign_jumps_:
> check_alignment_argument (loc, arg, "jumps");
> + // fix PR96247
> + if (0 == atoi(arg)) {
> + opts->x_flag_align_jumps = true;
> + opts->x_str_align_jumps = NULL;
> + }
> break;
>
> case OPT_falign_labels_:
> check_alignment_argument (loc, arg, "labels");
> + // fix PR96247
> + if (0 == atoi(arg)) {
> + opts->x_flag_align_labels = true;
> + opts->x_str_align_labels = NULL;
> + }
> break;
>
> case OPT_falign_functions_:
> check_alignment_argument (loc, arg, "functions");
> + // fix PR96247
> + if (0 == atoi(arg)) {
> + opts->x_flag_align_functions = true;
> + opts->x_str_align_functions = NULL;
> + }
> break;
>
> case OPT_ftabstop_:
> --
> 2.17.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]
2020-07-23 8:44 [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247] Hu Jiangping
2020-07-24 7:56 ` Hu, Jiangping
@ 2020-07-24 13:01 ` Segher Boessenkool
2020-07-24 15:50 ` Richard Sandiford
2 siblings, 0 replies; 5+ messages in thread
From: Segher Boessenkool @ 2020-07-24 13:01 UTC (permalink / raw)
To: Hu Jiangping; +Cc: gcc-patches
Hi!
Just some random comments...
On Thu, Jul 23, 2020 at 04:44:21PM +0800, Hu Jiangping wrote:
> + // fix PR96247
/* See PR96247. */
> + if (0 == atoi(arg)) {
Either
if (atoi (arg) == 0)
{
blalalala
or
if (!atoi (arg))
{
blalalala
(whichever reads best in this context).
> + // fix PR96247
Repeating that many times isn't helping the reader... It isn't
particularly useful even a single time, anyway? It is clear what this
does, and if anyone wants to see history, we have Git.
Segher
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]
2020-07-23 8:44 [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247] Hu Jiangping
2020-07-24 7:56 ` Hu, Jiangping
2020-07-24 13:01 ` Segher Boessenkool
@ 2020-07-24 15:50 ` Richard Sandiford
2020-07-27 7:37 ` Hu, Jiangping
2 siblings, 1 reply; 5+ messages in thread
From: Richard Sandiford @ 2020-07-24 15:50 UTC (permalink / raw)
To: Hu Jiangping; +Cc: gcc-patches
Hu Jiangping <hujiangping@cn.fujitsu.com> writes:
> Thanks, Richard!
>
> I think your suggestion is very good, so I made a new patch.
>
> v2: at a high level handles -falign-foo=0 like -falign-foo
> v1: at the target level overides the -falign-foo=0 option values
>
> Obviously, v2 is better than v1. In addition, anthor option
> to reject 0 that discussed in the email and PR96247
> is not as good as the current patch either, I think.
>
> I tested this patch on x86_64, it works well. OK for trunk?
In addition to Segher's comments, I wonder if it would be better
to pass &opts->x_flag_align_foo and &opts->x_str_align_jumps to
check_alignment_argument and do the check there instead.
The condition for whether to do this would then be:
align_result.length () == 1 && align_result[0] == 0
The reason for suggesting that is that it makes the parsing code
more self-consistent, rather than using atoi for this case only.
Looks good otherwise, thanks.
Richard
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247]
2020-07-24 15:50 ` Richard Sandiford
@ 2020-07-27 7:37 ` Hu, Jiangping
0 siblings, 0 replies; 5+ messages in thread
From: Hu, Jiangping @ 2020-07-27 7:37 UTC (permalink / raw)
To: Richard Sandiford, segher; +Cc: gcc-patches
> In addition to Segher's comments, I wonder if it would be better
> to pass &opts->x_flag_align_foo and &opts->x_str_align_jumps to
> check_alignment_argument and do the check there instead.
> The condition for whether to do this would then be:
>
> align_result.length () == 1 && align_result[0] == 0
>
> The reason for suggesting that is that it makes the parsing code
> more self-consistent, rather than using atoi for this case only.
>
Thanks, Segher and Richard!
I'll make a new patch to do the check in check_alignment_argument,
and change the condition of the if statement as follows:
align_result.length () >= 1 && align_result[0] == 0
for the input -falign-foo=n:m:n2:m2, according to documentation,
if n is zero, use the machine-dependent value. I think the implict
meaning is that even if m or n2 or m2 is specified, it should be
ignored. Any comments are appreciated!
> Looks good otherwise, thanks.
>
> Richard
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-07-27 7:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-23 8:44 [PATCH v2] driver: fix a problem with implementation of -falign-foo=0 [PR96247] Hu Jiangping
2020-07-24 7:56 ` Hu, Jiangping
2020-07-24 13:01 ` Segher Boessenkool
2020-07-24 15:50 ` Richard Sandiford
2020-07-27 7:37 ` Hu, Jiangping
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).