| Message ID | f3d6816a30a0782d83cb3aa6ae1cc5231b80cd29.1546385382.git.stefan@agner.ch |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: fix -masm-syntax-unified (PR88648) | expand |
Hi Stefan, On 01/01/19 23:34, Stefan Agner wrote: > This allows to use unified asm syntax when compiling for the > ARM instruction. This matches documentation and seems what the > initial patch was intended doing when the flag got added. > --- > gcc/config/arm/arm.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > index 3419b6bd0f8..67b2b199f3f 100644 > --- a/gcc/config/arm/arm.c > +++ b/gcc/config/arm/arm.c > @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, > > /* Thumb2 inline assembly code should always use unified syntax. > This will apply to ARM and Thumb1 eventually. */ > - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); > + if (TARGET_THUMB2_P (opts->x_target_flags)) > + opts->x_inline_asm_unified = true; This looks right to me and is the logic we had in GCC 5. How has this patch been tested? Can you please provide a ChangeLog entry for this patch[1]. Thanks, Kyrill [1] https://gcc.gnu.org/contribute.html > > #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS > SUBTARGET_OVERRIDE_INTERNAL_OPTIONS; > -- > 2.20.1 >
On 08/01/19 09:33, Kyrill Tkachov wrote: > Hi Stefan, > > On 01/01/19 23:34, Stefan Agner wrote: > > This allows to use unified asm syntax when compiling for the > > ARM instruction. This matches documentation and seems what the > > initial patch was intended doing when the flag got added. > > --- > > gcc/config/arm/arm.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > > index 3419b6bd0f8..67b2b199f3f 100644 > > --- a/gcc/config/arm/arm.c > > +++ b/gcc/config/arm/arm.c > > @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, > > > > /* Thumb2 inline assembly code should always use unified syntax. > > This will apply to ARM and Thumb1 eventually. */ > > - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); > > + if (TARGET_THUMB2_P (opts->x_target_flags)) > > + opts->x_inline_asm_unified = true; > > This looks right to me and is the logic we had in GCC 5. > How has this patch been tested? > For the avoidance of doubt, I mean that your patch is correct :) (not that the existing code is right). > Can you please provide a ChangeLog entry for this patch[1]. > > Thanks, > Kyrill > > [1] https://gcc.gnu.org/contribute.html > > > > > #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS > > SUBTARGET_OVERRIDE_INTERNAL_OPTIONS; > > -- > > 2.20.1 > > >
Hi Stefan, On 08/01/19 09:33, Kyrill Tkachov wrote: > Hi Stefan, > > On 01/01/19 23:34, Stefan Agner wrote: > > This allows to use unified asm syntax when compiling for the > > ARM instruction. This matches documentation and seems what the > > initial patch was intended doing when the flag got added. > > --- > > gcc/config/arm/arm.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > > index 3419b6bd0f8..67b2b199f3f 100644 > > --- a/gcc/config/arm/arm.c > > +++ b/gcc/config/arm/arm.c > > @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, > > > > /* Thumb2 inline assembly code should always use unified syntax. > > This will apply to ARM and Thumb1 eventually. */ > > - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); > > + if (TARGET_THUMB2_P (opts->x_target_flags)) > > + opts->x_inline_asm_unified = true; > > This looks right to me and is the logic we had in GCC 5. > How has this patch been tested? > > Can you please provide a ChangeLog entry for this patch[1]. > I've bootstrapped and tested this, together with your testsuite patch on arm-none-linux-gnueabihf and committed both with r267804 with the following ChangeLog entries: 2019-01-10 Stefan Agner <stefan@agner.ch> PR target/88648 * config/arm/arm.c (arm_option_override_internal): Force opts->x_inline_asm_unified to true only if TARGET_THUMB2_P. 2019-01-10 Stefan Agner <stefan@agner.ch> PR target/88648 * gcc.target/arm/pr88648-asm-syntax-unified.c: Add test to check if -masm-syntax-unified gets applied properly. Thank you for the patch. If you plan to contribute more patches in the future I suggest you sort out the copyright assignment paperwork. I believe this fix needs to be backported to the branches. I'll do so after a few days of testing on trunk. Thanks again, Kyrill > Thanks, > Kyrill > > [1] https://gcc.gnu.org/contribute.html > > > > > #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS > > SUBTARGET_OVERRIDE_INTERNAL_OPTIONS; > > -- > > 2.20.1 > > >
Hi Kyrill, On 10.01.2019 12:38, Kyrill Tkachov wrote: > Hi Stefan, > > On 08/01/19 09:33, Kyrill Tkachov wrote: >> Hi Stefan, >> >> On 01/01/19 23:34, Stefan Agner wrote: >> > This allows to use unified asm syntax when compiling for the >> > ARM instruction. This matches documentation and seems what the >> > initial patch was intended doing when the flag got added. >> > --- >> > gcc/config/arm/arm.c | 3 ++- >> > 1 file changed, 2 insertions(+), 1 deletion(-) >> > >> > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c >> > index 3419b6bd0f8..67b2b199f3f 100644 >> > --- a/gcc/config/arm/arm.c >> > +++ b/gcc/config/arm/arm.c >> > @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, >> > >> > /* Thumb2 inline assembly code should always use unified syntax. >> > This will apply to ARM and Thumb1 eventually. */ >> > - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); >> > + if (TARGET_THUMB2_P (opts->x_target_flags)) >> > + opts->x_inline_asm_unified = true; >> >> This looks right to me and is the logic we had in GCC 5. >> How has this patch been tested? >> >> Can you please provide a ChangeLog entry for this patch[1]. >> > > I've bootstrapped and tested this, together with your testsuite patch > on arm-none-linux-gnueabihf > and committed both with r267804 with the following ChangeLog entries: > > 2019-01-10 Stefan Agner <stefan@agner.ch> > > PR target/88648 > * config/arm/arm.c (arm_option_override_internal): Force > opts->x_inline_asm_unified to true only if TARGET_THUMB2_P. > > 2019-01-10 Stefan Agner <stefan@agner.ch> > > PR target/88648 > * gcc.target/arm/pr88648-asm-syntax-unified.c: Add test to > check if -masm-syntax-unified gets applied properly. > > Thank you for the patch. If you plan to contribute more patches in the > future I suggest you > sort out the copyright assignment paperwork. > > I believe this fix needs to be backported to the branches. > I'll do so after a few days of testing on trunk. Thanks for applying the patch! As far as I can see it did not made it into the branch yet, do you think it can get backported there too? -- Stefan > > Thanks again, > Kyrill > >> Thanks, >> Kyrill >> >> [1] https://gcc.gnu.org/contribute.html >> >> > >> > #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS >> > SUBTARGET_OVERRIDE_INTERNAL_OPTIONS; >> > -- >> > 2.20.1 >> > >>
Hi Stefan, On 09/02/19 16:25, Stefan Agner wrote: > Hi Kyrill, > > On 10.01.2019 12:38, Kyrill Tkachov wrote: >> Hi Stefan, >> >> On 08/01/19 09:33, Kyrill Tkachov wrote: >>> Hi Stefan, >>> >>> On 01/01/19 23:34, Stefan Agner wrote: >>>> This allows to use unified asm syntax when compiling for the >>>> ARM instruction. This matches documentation and seems what the >>>> initial patch was intended doing when the flag got added. >>>> --- >>>> gcc/config/arm/arm.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c >>>> index 3419b6bd0f8..67b2b199f3f 100644 >>>> --- a/gcc/config/arm/arm.c >>>> +++ b/gcc/config/arm/arm.c >>>> @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, >>>> >>>> /* Thumb2 inline assembly code should always use unified syntax. >>>> This will apply to ARM and Thumb1 eventually. */ >>>> - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); >>>> + if (TARGET_THUMB2_P (opts->x_target_flags)) >>>> + opts->x_inline_asm_unified = true; >>> This looks right to me and is the logic we had in GCC 5. >>> How has this patch been tested? >>> >>> Can you please provide a ChangeLog entry for this patch[1]. >>> >> I've bootstrapped and tested this, together with your testsuite patch >> on arm-none-linux-gnueabihf >> and committed both with r267804 with the following ChangeLog entries: >> >> 2019-01-10 Stefan Agner <stefan@agner.ch> >> >> PR target/88648 >> * config/arm/arm.c (arm_option_override_internal): Force >> opts->x_inline_asm_unified to true only if TARGET_THUMB2_P. >> >> 2019-01-10 Stefan Agner <stefan@agner.ch> >> >> PR target/88648 >> * gcc.target/arm/pr88648-asm-syntax-unified.c: Add test to >> check if -masm-syntax-unified gets applied properly. >> >> Thank you for the patch. If you plan to contribute more patches in the >> future I suggest you >> sort out the copyright assignment paperwork. >> >> I believe this fix needs to be backported to the branches. >> I'll do so after a few days of testing on trunk. > Thanks for applying the patch! As far as I can see it did not made it > into the branch yet, do you think it can get backported there too? Thanks for reminding me. I've now committed your patch and the test to the gcc-8 branch with r268764. Kyrill > -- > Stefan > >> Thanks again, >> Kyrill >> >>> Thanks, >>> Kyrill >>> >>> [1] https://gcc.gnu.org/contribute.html >>> >>>> #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS >>>> SUBTARGET_OVERRIDE_INTERNAL_OPTIONS; >>>> -- >>>> 2.20.1 >>>>
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 3419b6bd0f8..67b2b199f3f 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -3095,7 +3095,8 @@ arm_option_override_internal (struct gcc_options *opts, /* Thumb2 inline assembly code should always use unified syntax. This will apply to ARM and Thumb1 eventually. */ - opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags); + if (TARGET_THUMB2_P (opts->x_target_flags)) + opts->x_inline_asm_unified = true; #ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS SUBTARGET_OVERRIDE_INTERNAL_OPTIONS;