[PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Jozef Lawrynowicz]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

The attached patch allows the case of register names used in an asm statement
clobber list to be disregarded when checking the validity of the register names.

Currently, the register name used in asm statement clobber list must exactly
match those defined in the targets REGISTER_NAMES macro.

This means that, for example, for msp430-elf the following code emits an
ambiguous error:

> void
> foo (void)
> {
>   __asm__ ("" : : : "r4", "R6");
> }

> asm-register-names.c:8:3: error: unknown register name 'r4' in 'asm'

All the register names defined in the msp430 REGISTER_NAMES macro use an
upper case 'R', so use of lower case 'r' gets rejected.

I guess this could also be fixed by defining all the registers in
ADDITIONAL_REGISTER_NAMES using a lower case r, but I prefer this generic
solution and I cannot think of any negative side effects to what is proposed.
Using the wrong case of a register name does not make its use ambiguous, and
allows users to stylize the clobber list with upper/lower case register names
as they wish.

Successfully bootstrapped and regtested for x86_64-pc-linux-gnu, and regtested
for msp430-elf.

Ok for trunk?

[-- Attachment #2: 0001-Perform-case-insensitive-comparison-when-decoding-re.patch --]
[-- Type: text/x-patch, Size: 2799 bytes --]

From 6275eb1c915b574f415c4adaf241d2d200c42cce Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Thu, 4 Jul 2019 11:25:04 +0100
Subject: [PATCH] Perform case-insensitive comparison when decoding register
 names

gcc/ChangeLog:

2019-07-04  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	PR target/70320
	* varasm.c (decode_reg_name_and_count): Use strcasecmp when comparing
	register names with asmspec.

gcc/testsuite/ChangeLog:

2019-07-04  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	PR target/70320
	* gcc.target/msp430/asm-register-names.c: New test.
---
 .../gcc.target/msp430/asm-register-names.c          | 13 +++++++++++++
 gcc/varasm.c                                        | 10 +++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/asm-register-names.c

diff --git a/gcc/testsuite/gcc.target/msp430/asm-register-names.c b/gcc/testsuite/gcc.target/msp430/asm-register-names.c
new file mode 100644
index 00000000000..5c31c23d9db
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/asm-register-names.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler "PUSH.*R4" } } */
+/* { dg-final { scan-assembler "PUSH.*R6" } } */
+
+/* PR target/70320
+   Check that both lower and upper case "r" is accepted in asm statement
+   clobber list.  */
+
+void
+foo (void)
+{
+  __asm__ ("" : : : "r4", "R6");
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index 626a4c9f9a0..892f678e9e9 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -947,7 +947,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)
 
       for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
 	if (reg_names[i][0]
-	    && ! strcmp (asmspec, strip_reg_name (reg_names[i])))
+	    && ! strcasecmp (asmspec, strip_reg_name (reg_names[i])))
 	  return i;
 
 #ifdef OVERLAPPING_REGISTER_NAMES
@@ -961,7 +961,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)
 
 	for (i = 0; i < (int) ARRAY_SIZE (table); i++)
 	  if (table[i].name[0]
-	      && ! strcmp (asmspec, table[i].name))
+	      && ! strcasecmp (asmspec, table[i].name))
 	    {
 	      *pnregs = table[i].nregs;
 	      return table[i].number;
@@ -976,16 +976,16 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)
 
 	for (i = 0; i < (int) ARRAY_SIZE (table); i++)
 	  if (table[i].name[0]
-	      && ! strcmp (asmspec, table[i].name)
+	      && ! strcasecmp (asmspec, table[i].name)
 	      && reg_names[table[i].number][0])
 	    return table[i].number;
       }
 #endif /* ADDITIONAL_REGISTER_NAMES */
 
-      if (!strcmp (asmspec, "memory"))
+      if (!strcasecmp (asmspec, "memory"))
 	return -4;
 
-      if (!strcmp (asmspec, "cc"))
+      if (!strcasecmp (asmspec, "cc"))
 	return -3;
 
       return -2;
-- 
2.17.1

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Segher Boessenkool]

On Thu, Jul 04, 2019 at 01:32:59PM +0100, Jozef Lawrynowicz wrote:
> The attached patch allows the case of register names used in an asm statement
> clobber list to be disregarded when checking the validity of the register names.
> 
> Currently, the register name used in asm statement clobber list must exactly
> match those defined in the targets REGISTER_NAMES macro.

> All the register names defined in the msp430 REGISTER_NAMES macro use an
> upper case 'R', so use of lower case 'r' gets rejected.
> 
> I guess this could also be fixed by defining all the registers in
> ADDITIONAL_REGISTER_NAMES using a lower case r, but I prefer this generic
> solution and I cannot think of any negative side effects to what is proposed.

It isn't obviously safe either.  Are there any targets that have names
for different registers that differ only in case?  You could say that
such a design deserves what is coming for it, but :-)

> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -947,7 +947,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)

This is used for more than just clobber lists.  Is this change safe, and
a good thing, in the other contexts where it changes things?

> -      if (!strcmp (asmspec, "memory"))
> +      if (!strcasecmp (asmspec, "memory"))
>  	return -4;
>  
> -      if (!strcmp (asmspec, "cc"))
> +      if (!strcasecmp (asmspec, "cc"))
>  	return -3;

You don't need to change these.


Segher

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Thu, Jul 04, 2019 at 01:32:59PM +0100, Jozef Lawrynowicz wrote:
>> The attached patch allows the case of register names used in an asm statement
>> clobber list to be disregarded when checking the validity of the register names.
>> 
>> Currently, the register name used in asm statement clobber list must exactly
>> match those defined in the targets REGISTER_NAMES macro.
>
>> All the register names defined in the msp430 REGISTER_NAMES macro use an
>> upper case 'R', so use of lower case 'r' gets rejected.
>> 
>> I guess this could also be fixed by defining all the registers in
>> ADDITIONAL_REGISTER_NAMES using a lower case r, but I prefer this generic
>> solution and I cannot think of any negative side effects to what is proposed.
>
> It isn't obviously safe either.  Are there any targets that have names
> for different registers that differ only in case?  You could say that
> such a design deserves what is coming for it, but :-)
>
>> --- a/gcc/varasm.c
>> +++ b/gcc/varasm.c
>> @@ -947,7 +947,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)
>
> This is used for more than just clobber lists.  Is this change safe, and
> a good thing, in the other contexts where it changes things?
>
>> -      if (!strcmp (asmspec, "memory"))
>> +      if (!strcasecmp (asmspec, "memory"))
>>  	return -4;
>>  
>> -      if (!strcmp (asmspec, "cc"))
>> +      if (!strcasecmp (asmspec, "cc"))
>>  	return -3;
>
> You don't need to change these.

Agreed.  There's also the problem that if we make this available for
all targets now, people might start using it without realising that
it implicitly requires GCC 10+.

But having the feature opt-in (by a DEFHOOKPOD?) sounds good, and
definitely more maintainable than having to add aliases in the
target code.

Thanks,
Richard

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Jozef Lawrynowicz]

On Mon, 08 Jul 2019 21:14:36 +0100
Richard Sandiford <richard.sandiford@arm.com> wrote:

> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Thu, Jul 04, 2019 at 01:32:59PM +0100, Jozef Lawrynowicz wrote:  
> >> The attached patch allows the case of register names used in an asm statement
> >> clobber list to be disregarded when checking the validity of the register names.
> >> 
> >> Currently, the register name used in asm statement clobber list must exactly
> >> match those defined in the targets REGISTER_NAMES macro.  
> >  
> >> All the register names defined in the msp430 REGISTER_NAMES macro use an
> >> upper case 'R', so use of lower case 'r' gets rejected.
> >> 
> >> I guess this could also be fixed by defining all the registers in
> >> ADDITIONAL_REGISTER_NAMES using a lower case r, but I prefer this generic
> >> solution and I cannot think of any negative side effects to what is proposed.  
> >
> > It isn't obviously safe either.  Are there any targets that have names
> > for different registers that differ only in case?  You could say that
> > such a design deserves what is coming for it, but :-)

Indeed, I did have a read through all the definitions of REGISTER_NAMES in the
gcc/config and could not spot any cases where different register nanes differed
only in their case. I didn't check it programmatically though, so it's
not impossible I missed something..

> >  
> >> --- a/gcc/varasm.c
> >> +++ b/gcc/varasm.c
> >> @@ -947,7 +947,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)  
> >
> > This is used for more than just clobber lists.  Is this change safe, and
> > a good thing, in the other contexts where it changes things?

It appears to be used for only two purposes (mostly via the
"decode_reg_name" wrapper):
- Decoding the register name in an asm spec and reporting any misuse
- Decoding parameters passed to command line options
Generic options using it are -fcall-used/saved-REG and -ffixed-REG
-fstack-limit-register.
Backends use it for target specific options such as -mfixed-range= for SPU.
Apart from that there appears to be a single other use of it in make_decl_rtl
to report when "register name given for non-register variable", although I
could not immediately reproduce this myself to understand this specific case it
is triggered for.

> >  
> >> -      if (!strcmp (asmspec, "memory"))
> >> +      if (!strcasecmp (asmspec, "memory"))
> >>  	return -4;
> >>  
> >> -      if (!strcmp (asmspec, "cc"))
> >> +      if (!strcasecmp (asmspec, "cc"))
> >>  	return -3;  
> >
> > You don't need to change these.  
> 
> Agreed.  There's also the problem that if we make this available for
> all targets now, people might start using it without realising that
> it implicitly requires GCC 10+.
> 
> But having the feature opt-in (by a DEFHOOKPOD?) sounds good, and
> definitely more maintainable than having to add aliases in the
> target code.
> 
> Thanks,
> Richard

Ok, yes a DEFHOOKPOD or similar sounds like a good idea, I'll look into this
alternative.

Thanks,
Jozef

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Segher Boessenkool]

On Mon, Jul 08, 2019 at 10:21:29PM +0100, Jozef Lawrynowicz wrote:
> On Mon, 08 Jul 2019 21:14:36 +0100
> Richard Sandiford <richard.sandiford@arm.com> wrote:
> 
> > Segher Boessenkool <segher@kernel.crashing.org> writes:
> > > It isn't obviously safe either.  Are there any targets that have names
> > > for different registers that differ only in case?  You could say that
> > > such a design deserves what is coming for it, but :-)
> 
> Indeed, I did have a read through all the definitions of REGISTER_NAMES in the
> gcc/config and could not spot any cases where different register nanes differed
> only in their case. I didn't check it programmatically though, so it's
> not impossible I missed something..

You also haven't checked future GCC versions, for future processors ;-)
And, not all out-of-tree ports, either.  Gratuitously breaking those
isn't ideal.

> > >> --- a/gcc/varasm.c
> > >> +++ b/gcc/varasm.c
> > >> @@ -947,7 +947,7 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)  
> > >
> > > This is used for more than just clobber lists.  Is this change safe, and
> > > a good thing, in the other contexts where it changes things?
> 
> It appears to be used for only two purposes (mostly via the
> "decode_reg_name" wrapper):
> - Decoding the register name in an asm spec and reporting any misuse
> - Decoding parameters passed to command line options
> Generic options using it are -fcall-used/saved-REG and -ffixed-REG
> -fstack-limit-register.
> Backends use it for target specific options such as -mfixed-range= for SPU.
> Apart from that there appears to be a single other use of it in make_decl_rtl
> to report when "register name given for non-register variable", although I
> could not immediately reproduce this myself to understand this specific case it
> is triggered for.

It is used for register asm, yes.  This is e.g.

void f(int x)
{
	int y asm("r10");
	y = x;
	asm ("# %0" :: "r"(y));
}

which complains

  warning: ignoring 'asm' specifier for non-static local variable 'y'

(Making the declaration of y static does nothing, doesn't make it use r10
that is; adding "register" does though).

> Ok, yes a DEFHOOKPOD or similar sounds like a good idea, I'll look into this
> alternative.

What is that, like target macros?  But with some indirection?

Making this target-specific sounds good, thanks Jozef.


Segher

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Jozef Lawrynowicz]

On Mon, 8 Jul 2019 16:42:15 -0500
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> > Ok, yes a DEFHOOKPOD or similar sounds like a good idea, I'll look into this
> > alternative.  
> 
> What is that, like target macros?  But with some indirection?

Yes its for target macros, it looks like the "POD" in DEFHOOKPOD stands for
"piece-of-data", i.e. the hook represents a variable rather than function.

So we can just have a hook like TARGET_CASE_INSENSITIVE_REGISTER_NAME set to
false by default, and then check targetm.case_insensitive_register_name before
comparing the given regname with the names defined by the backend.

> 
> Making this target-specific sounds good, thanks Jozef.
> 
> 
> Segher

Thanks,
Jozef

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Segher Boessenkool]

On Tue, Jul 09, 2019 at 10:16:31PM +0100, Jozef Lawrynowicz wrote:
> On Mon, 8 Jul 2019 16:42:15 -0500
> Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> > > Ok, yes a DEFHOOKPOD or similar sounds like a good idea, I'll look into this
> > > alternative.  
> > 
> > What is that, like target macros?  But with some indirection?
> 
> Yes its for target macros, it looks like the "POD" in DEFHOOKPOD stands for
> "piece-of-data", i.e. the hook represents a variable rather than function.

But it is data, not a constant, so it does not allow optimising based
on its potentially constant value?  Where "potentially" in this case
means "always" :-/


Segher

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Jozef Lawrynowicz]

On Tue, 9 Jul 2019 16:36:46 -0500
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> On Tue, Jul 09, 2019 at 10:16:31PM +0100, Jozef Lawrynowicz wrote:
> > On Mon, 8 Jul 2019 16:42:15 -0500
> > Segher Boessenkool <segher@kernel.crashing.org> wrote:
> >   
> > > > Ok, yes a DEFHOOKPOD or similar sounds like a good idea, I'll look into this
> > > > alternative.    
> > > 
> > > What is that, like target macros?  But with some indirection?  
> > 
> > Yes its for target macros, it looks like the "POD" in DEFHOOKPOD stands for
> > "piece-of-data", i.e. the hook represents a variable rather than function.  
> 
> But it is data, not a constant, so it does not allow optimising based
> on its potentially constant value?  Where "potentially" in this case
> means "always" :-/
> 
> 
> Segher

I can think of two alternatives so far which should get around this
optimization issue you point out.

1. a regular target macro defined in tm.texi such as
TARGET_CASE_INSENSITIVE_REGISTER_NAMES, defined to 0 by default. Overriden
with 1 for MSP430 

>         for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
>         if (reg_names[i][0]
> +#if TARGET_CASE_INSENSITIVE_REGISTER_NAMES
> +           && ! strcasecmp (asmspec, strip_reg_name (reg_names[i])))
> +#else
>             && ! strcmp (asmspec, strip_reg_name (reg_names[i])))
> +#endif
>           return i;

2. a function-like target macro TARGET_COMPARE_REGISTER_NAMES in tm.texi which
defines the function to use for register name comparisons. Defined to
"strcmp" by default and overriden with strcasecmp for MSP430.

>         for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
>         if (reg_names[i][0]
> -           && ! strcmp (asmspec, strip_reg_name (reg_names[i])))
> +           && ! TARGET_COMPARE_REGISTER_NAMES (asmspec, strip_reg_name (reg_names[i])))
>          return i;

Alternatively a DEFHOOK could be used for above so we'd have
targetm.compare_register_names (asmspec, ...) which would essentially be a
wrapper round strcmp or strcasecmp. But I'm unsure if GCC would end up
inlining the str{,case}cmp call from the target hook - maybe if the file is
built with lto.. So we may end up with sub-optimal code again with this.

I think (1) is more immediately clear as to what the macro is doing, although
it is less concise than (2) as 3 of these #if..else blocks would be required.
Nevertheless (1) would still be my preferred solution.
Any thoughts?

Thanks,
Jozef

Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)

[Segher Boessenkool]

On Wed, Jul 10, 2019 at 05:23:28PM +0100, Jozef Lawrynowicz wrote:
> On Tue, 9 Jul 2019 16:36:46 -0500
> Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > But it is data, not a constant, so it does not allow optimising based
> > on its potentially constant value?  Where "potentially" in this case
> > means "always" :-/
> 
> I can think of two alternatives so far which should get around this
> optimization issue you point out.
> 
> 1. a regular target macro defined in tm.texi such as
> TARGET_CASE_INSENSITIVE_REGISTER_NAMES, defined to 0 by default. Overriden
> with 1 for MSP430 

That is how most other things work, yeah.

For this particular case the impact of using DEFHOOKPOD instead of a
target macro would be minimal, of course, register name compare are not
executed so very often; but we cannot convert all the other target
macros this way.


Segher