[PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

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

The attached patch adds a new target macro called 
CASE_INSENSITIVE_REGISTER_NAMES, which allows the case of register names
used in an asm statement clobber list, or given in a command line option, to be
disregarded when comparing with the register names defined for the target in
REGISTER_NAMES. 

The macro is set to 1 for msp430 only, and set to 0 by default, so comparisons
continue to be case-sensitive for all targets except msp430.

Previously, a register name provided by the user using one of the aforementioned
methods 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.

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

Ok for trunk?

[-- Attachment #2: 0001-Implement-CASE_INSENSITIVE_REGISTER_NAMES.patch --]
[-- Type: text/x-patch, Size: 6482 bytes --]

From 82eadcdcbb8914b06818f7c8a10156336518e8d1 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Wed, 17 Jul 2019 11:48:23 +0100
Subject: [PATCH] Implement CASE_INSENSITIVE_REGISTER_NAMES

gcc/ChangeLog:

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

	PR target/70320
	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
	* doc/tm.texi: Likewise.
	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
	strcmp for comparisons of asmspec with a register name if 
	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.

gcc/testsuite/ChangeLog:

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

	PR target/70320
	* gcc.target/msp430/asm-register-names.c: New test. 
---
 gcc/config/msp430/msp430.h                    |  1 +
 gcc/defaults.h                                |  4 ++++
 gcc/doc/tm.texi                               | 19 +++++++++++++++++++
 gcc/doc/tm.texi.in                            | 19 +++++++++++++++++++
 .../gcc.target/msp430/asm-register-names.c    | 14 ++++++++++++++
 gcc/varasm.c                                  | 18 ++++++++++++++++--
 6 files changed, 73 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/asm-register-names.c

diff --git a/gcc/config/msp430/msp430.h b/gcc/config/msp430/msp430.h
index 1288b1a263d..7b02c5fe28d 100644
--- a/gcc/config/msp430/msp430.h
+++ b/gcc/config/msp430/msp430.h
@@ -317,6 +317,7 @@ enum reg_class
 #define REGNO_OK_FOR_BASE_P(regno)	1
 #define REGNO_OK_FOR_INDEX_P(regno)	1
 
+#define CASE_INSENSITIVE_REGISTER_NAMES 1
 \f
 
 typedef struct
diff --git a/gcc/defaults.h b/gcc/defaults.h
index af7ea185f1e..2a22d52ba2f 100644
--- a/gcc/defaults.h
+++ b/gcc/defaults.h
@@ -1254,6 +1254,10 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #define SHORT_IMMEDIATES_SIGN_EXTEND 0
 #endif
 
+#ifndef CASE_INSENSITIVE_REGISTER_NAMES
+#define CASE_INSENSITIVE_REGISTER_NAMES 0
+#endif
+
 #ifndef WORD_REGISTER_OPERATIONS
 #define WORD_REGISTER_OPERATIONS 0
 #endif
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 8e5b01c9383..b895dfaa4b0 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -2000,6 +2000,25 @@ If the program counter has a register number, define this as that
 register number.  Otherwise, do not define it.
 @end defmac
 
+@defmac CASE_INSENSITIVE_REGISTER_NAMES
+Define this macro to 1 if it is safe to disregard the case of register
+names when comparing user-provided register names with the
+names defined by @code{REGISTER_NAMES}.  By default this is set to
+0.
+
+This affects the register clobber list in an @code{asm} statement and
+command line options which accept register names, such as
+@option{-ffixed-@var{reg}}.
+
+For example, if @code{REGISTER_NAMES} defines a register called @var{R4},
+then the following use of lower case @var{r4} will not result in an error
+if this macro is defined to 1.
+@smallexample
+asm ("" : : : "r4");
+@end smallexample
+
+@end defmac
+
 @node Allocation Order
 @subsection Order of Allocation of Registers
 @cindex order of register allocation
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index b4d57b86e2f..4aba454248e 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -1749,6 +1749,25 @@ If the program counter has a register number, define this as that
 register number.  Otherwise, do not define it.
 @end defmac
 
+@defmac CASE_INSENSITIVE_REGISTER_NAMES
+Define this macro to 1 if it is safe to disregard the case of register
+names when comparing user-provided register names with the
+names defined by @code{REGISTER_NAMES}.  By default this is set to
+0.
+
+This affects the register clobber list in an @code{asm} statement and
+command line options which accept register names, such as
+@option{-ffixed-@var{reg}}.
+
+For example, if @code{REGISTER_NAMES} defines a register called @var{R4},
+then the following use of lower case @var{r4} will not result in an error
+if this macro is defined to 1.
+@smallexample
+asm ("" : : : "r4");
+@end smallexample
+
+@end defmac
+
 @node Allocation Order
 @subsection Order of Allocation of Registers
 @cindex order of register allocation
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..3a963eb61d9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/asm-register-names.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-ffixed-r6 -ffixed-R7" } */
+/* { dg-final { scan-assembler "PUSH.*R4" } } */
+/* { dg-final { scan-assembler "PUSH.*R5" } } */
+
+/* PR target/70320
+   Check that both lower and upper case "r" in register names is accepted in
+   an asm statement clobber list and as arguments to command line options.  */
+
+void
+foo (void)
+{
+  __asm__ ("" : : : "r4", "R5");
+}
diff --git a/gcc/varasm.c b/gcc/varasm.c
index e886cdc71b8..ab04bc2c332 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -947,7 +947,12 @@ 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])))
+#if CASE_INSENSITIVE_REGISTER_NAMES
+	    && ! strcasecmp (asmspec, strip_reg_name (reg_names[i]))
+#else
+	    && ! strcmp (asmspec, strip_reg_name (reg_names[i]))
+#endif /* CASE_INSENSITIVE_REGISTER_NAMES */
+	)
 	  return i;
 
 #ifdef OVERLAPPING_REGISTER_NAMES
@@ -961,7 +966,12 @@ 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))
+#if CASE_INSENSITIVE_REGISTER_NAMES
+	      && ! strcasecmp (asmspec, table[i].name)
+#else
+	      && ! strcmp (asmspec, table[i].name)
+#endif /* CASE_INSENSITIVE_REGISTER_NAMES */
+	  )
 	    {
 	      *pnregs = table[i].nregs;
 	      return table[i].number;
@@ -976,7 +986,11 @@ decode_reg_name_and_count (const char *asmspec, int *pnregs)
 
 	for (i = 0; i < (int) ARRAY_SIZE (table); i++)
 	  if (table[i].name[0]
+#if CASE_INSENSITIVE_REGISTER_NAMES
+	      && ! strcasecmp (asmspec, table[i].name)
+#else
 	      && ! strcmp (asmspec, table[i].name)
+#endif /* CASE_INSENSITIVE_REGISTER_NAMES */
 	      && reg_names[table[i].number][0])
 	    return table[i].number;
       }
-- 
2.17.1

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Segher Boessenkool]

Hi!

On Thu, Jul 18, 2019 at 08:45:38PM +0100, Jozef Lawrynowicz wrote:
> 	PR target/70320
> 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> 	* doc/tm.texi: Likewise.

"Regenerate." -- or did you edit this file by hand?  Don't, or don't tell
us anyway ;-)

> 	strcmp for comparisons of asmspec with a register name if 

(Trailing space here, and elsewhere).

> +/* { dg-do compile } */
> +/* { dg-options "-ffixed-r6 -ffixed-R7" } */
> +/* { dg-final { scan-assembler "PUSH.*R4" } } */
> +/* { dg-final { scan-assembler "PUSH.*R5" } } */

scan-assembler does multi-line matching by default, so that .* probably
matches things you do not want it to match.  You can do things like

/* { dg-final { scan-assembler "(?n)PUSH.*R5" } } */

to make sure this is on one line at least.  See man re_syntax.

Rest looks fine, but I'm not an RTL maintainer.


Segher

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

On Thu, 18 Jul 2019 16:55:59 -0500
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!

Hi Segher,

> 
> On Thu, Jul 18, 2019 at 08:45:38PM +0100, Jozef Lawrynowicz wrote:
> > 	PR target/70320
> > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> > 	* doc/tm.texi: Likewise.  
> 
> "Regenerate." -- or did you edit this file by hand?  Don't, or don't tell
> us anyway ;-)

I did indeed regenerate it, that's just a mistake in the ChangeLog.
> 
> > 	strcmp for comparisons of asmspec with a register name if   
> 
> (Trailing space here, and elsewhere).

Ah, I was given false confidence by contrib/check_GNU_style.sh, I guess that
only checks +/- lines.

> 
> > +/* { dg-do compile } */
> > +/* { dg-options "-ffixed-r6 -ffixed-R7" } */
> > +/* { dg-final { scan-assembler "PUSH.*R4" } } */
> > +/* { dg-final { scan-assembler "PUSH.*R5" } } */  
> 
> scan-assembler does multi-line matching by default, so that .* probably
> matches things you do not want it to match.  You can do things like
> 
> /* { dg-final { scan-assembler "(?n)PUSH.*R5" } } */
> 
> to make sure this is on one line at least.  See man re_syntax.

Right, thanks for pointing that out, the test was in fact matching cases it
shouldn't have been due to the multi-line matching.

Thanks for the review,
Jozef

> 
> Rest looks fine, but I'm not an RTL maintainer.
> 
> 
> Segher

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Richard Sandiford]

Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> The attached patch adds a new target macro called 
> CASE_INSENSITIVE_REGISTER_NAMES, which allows the case of register names
> used in an asm statement clobber list, or given in a command line option, to be
> disregarded when comparing with the register names defined for the target in
> REGISTER_NAMES. 
>
> The macro is set to 1 for msp430 only, and set to 0 by default, so
> comparisons continue to be case-sensitive for all targets except
> msp430.
>
> Previously, a register name provided by the user using one of the
> aforementioned methods 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.
>
> Successfully bootstrapped and regtested on trunk for x86_64-pc-linux-gnu, and
> regtested for msp430-elf.
>
> Ok for trunk?
>
> From 82eadcdcbb8914b06818f7c8a10156336518e8d1 Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> Date: Wed, 17 Jul 2019 11:48:23 +0100
> Subject: [PATCH] Implement CASE_INSENSITIVE_REGISTER_NAMES
>
> gcc/ChangeLog:
>
> 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
>
> 	PR target/70320
> 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> 	* doc/tm.texi: Likewise.
> 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
> 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
> 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
> 	strcmp for comparisons of asmspec with a register name if 
> 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.

I really don't think we should be adding new target macros for things
like this.  The code is hardly on the critical path, so I don't think
compile time is a concern.  That said...

> diff --git a/gcc/varasm.c b/gcc/varasm.c
> index e886cdc71b8..ab04bc2c332 100644
> --- a/gcc/varasm.c
> +++ b/gcc/varasm.c
> @@ -947,7 +947,12 @@ 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])))
> +#if CASE_INSENSITIVE_REGISTER_NAMES
> +	    && ! strcasecmp (asmspec, strip_reg_name (reg_names[i]))
> +#else
> +	    && ! strcmp (asmspec, strip_reg_name (reg_names[i]))
> +#endif /* CASE_INSENSITIVE_REGISTER_NAMES */
> +	)
>  	  return i;
>  
>  #ifdef OVERLAPPING_REGISTER_NAMES

...if we do keep it as a macro, we should use:

	if (reg_names[i][0]
	    && (CASE_INSENSITIVE_REGISTER_NAMES
		? !strcasecmp (asmspec, strip_reg_name (reg_names[i]))
		: !strcmp (asmspec, strip_reg_name (reg_names[i]))))

So TBH I still prefer the DEFHOOKPOD suggestion.  I won't object if
someone else wants to approve the macro version though.

Thanks,
Richard

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

On Fri, 19 Jul 2019 09:31:15 +0100
Richard Sandiford <richard.sandiford@arm.com> wrote:

> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> >
> > From 82eadcdcbb8914b06818f7c8a10156336518e8d1 Mon Sep 17 00:00:00 2001
> > From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> > Date: Wed, 17 Jul 2019 11:48:23 +0100
> > Subject: [PATCH] Implement CASE_INSENSITIVE_REGISTER_NAMES
> >
> > gcc/ChangeLog:
> >
> > 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> >
> > 	PR target/70320
> > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> > 	* doc/tm.texi: Likewise.
> > 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
> > 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
> > 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
> > 	strcmp for comparisons of asmspec with a register name if 
> > 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.  
> 
> I really don't think we should be adding new target macros for things
> like this.  The code is hardly on the critical path, so I don't think
> compile time is a concern.  That said...
> 
> ...
> 
> So TBH I still prefer the DEFHOOKPOD suggestion.  I won't object if
> someone else wants to approve the macro version though.

Ok, as you say, this code isn't on the critical path so I'd be happy to change
this to a DEFHOOKPOD.

In general, what should be considered when deciding between a hook and macro?
Does the choice lean towards macros mainly when compile time is a concern? And
hooks otherwise?

Is the downside of this macro implementation compared to a DEFHOOKPOD mainly
just the maintainability/readability of the added code?

Since if I take your welcome recommendation for how to improve the macro
implementation -

> ...if we do keep it as a macro, we should use:
> 
> 	if (reg_names[i][0]
> 	    && (CASE_INSENSITIVE_REGISTER_NAMES
> 		? !strcasecmp (asmspec, strip_reg_name (reg_names[i]))
> 		: !strcmp (asmspec, strip_reg_name (reg_names[i]))))

then when I make it a DEFHOOKPOD the changes in this function will look the
save as above, except CASE_INSENSITIVE_REGISTER_NAMES will be
targetm.case_insensitive_register_names.

Thanks,
Jozef

> 
> Thanks,
> Richard

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jakub Jelinek]

On Fri, Jul 19, 2019 at 10:39:52AM +0100, Jozef Lawrynowicz wrote:
> > > 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > >
> > > 	PR target/70320
> > > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> > > 	* doc/tm.texi: Likewise.
> > > 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
> > > 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
> > > 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
> > > 	strcmp for comparisons of asmspec with a register name if 
> > > 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.  

Ugh, do we really need this?  If it is just for msp430, can't it instead
just
#define ADDITIONAL_REGISTER_NAMES macro and add those 16 "rN" register name
aliases to the current REGISTER_NAMES "RN" names?

	Jakub

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

On Fri, 19 Jul 2019 11:54:43 +0200
Jakub Jelinek <jakub@redhat.com> wrote:

> On Fri, Jul 19, 2019 at 10:39:52AM +0100, Jozef Lawrynowicz wrote:
> > > > 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> > > >
> > > > 	PR target/70320
> > > > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> > > > 	* doc/tm.texi: Likewise.
> > > > 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
> > > > 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
> > > > 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
> > > > 	strcmp for comparisons of asmspec with a register name if 
> > > > 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.    
> 
> Ugh, do we really need this?  If it is just for msp430, can't it instead
> just
> #define ADDITIONAL_REGISTER_NAMES macro and add those 16 "rN" register name
> aliases to the current REGISTER_NAMES "RN" names?
> 
> 	Jakub

That is something I considered in previous discussion here:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00372.html

But it seemed like this could potentially be a useful feature for other targets
that wish to enable it. It doesn't appear necessary to restrict the case of
register names, unless a target exists that has different registers that differ
only by case.

It makes the programmer's life easier IMO and gives them more choice as to how
they style their code. You can already use the register number alone without a
prefix (i.e. "9" instead of "r9"), this seems like a natural extension to that
flexibility.

As far as I can tell the exact register names for the different targets aren't
documented anywhere either. The MSP430 ABI, at least, doesn't explicitly
enforce a case for the register names. I would imagine there are others that
don't either.

Jozef

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jakub Jelinek]

On Fri, Jul 19, 2019 at 11:17:51AM +0100, Jozef Lawrynowicz wrote:
> That is something I considered in previous discussion here:
> https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00372.html
> 
> But it seemed like this could potentially be a useful feature for other targets
> that wish to enable it. It doesn't appear necessary to restrict the case of
> register names, unless a target exists that has different registers that differ
> only by case.

It doesn't seem like a generally useful feature to me, C as well as C++ are
case sensitive, so is gcc command line parsing, so having the register names
handled insensitive is strange and undesirable.
Perhaps the reason you want it for msp430 is that the register names were
chosen badly as upper case which surprises people?
Having register int x __asm ("eAx"); register __m512i __asm ("ZmM11"); is simply
weird, not something we should allow nor promote.

	Jakub

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

On Fri, 19 Jul 2019 12:24:57 +0200
Jakub Jelinek <jakub@redhat.com> wrote:

> On Fri, Jul 19, 2019 at 11:17:51AM +0100, Jozef Lawrynowicz wrote:
> > That is something I considered in previous discussion here:
> > https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00372.html
> > 
> > But it seemed like this could potentially be a useful feature for other targets
> > that wish to enable it. It doesn't appear necessary to restrict the case of
> > register names, unless a target exists that has different registers that differ
> > only by case.  
> 
> It doesn't seem like a generally useful feature to me, C as well as C++ are
> case sensitive, so is gcc command line parsing, so having the register names
> handled insensitive is strange and undesirable.

That's true, but I guess user's feel that they are in assembly language "mode"
rather than C mode, since the clobber list is a string adjacent to a string of
assembler code.
The GNU assembler considers
> "Upper and lower case are equivalent in register names, opcodes, condition 
> codes and assembler directives."
(https://sourceware.org/binutils/docs/as/Z80_002dCase.html#Z80_002dCase)

> Perhaps the reason you want it for msp430 is that the register names were
> chosen badly as upper case which surprises people?

This is true. Users probably would not have complained and I would not be
considering this issue if the REGISTER_NAMES were originally lower case.

> Having register int x __asm ("eAx"); register __m512i __asm ("ZmM11"); is simply
> weird, not something we should allow nor promote.

I was thinking along the lines that someone might want "EAX" rather than "eax".

At this point though, unless someone chimes in and says that they think
case-insensitive names would be useful for their target and they want this
hook, I will just go with the less controversial option and implement this
using ADDITIONAL_REGISTER_NAMES for msp430 only.

Thanks for the feedback,
Jozef

> 
> 	Jakub

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Richard Sandiford]

Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> On Fri, 19 Jul 2019 09:31:15 +0100
> Richard Sandiford <richard.sandiford@arm.com> wrote:
>
>> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
>> >
>> > From 82eadcdcbb8914b06818f7c8a10156336518e8d1 Mon Sep 17 00:00:00 2001
>> > From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
>> > Date: Wed, 17 Jul 2019 11:48:23 +0100
>> > Subject: [PATCH] Implement CASE_INSENSITIVE_REGISTER_NAMES
>> >
>> > gcc/ChangeLog:
>> >
>> > 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
>> >
>> > 	PR target/70320
>> > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
>> > 	* doc/tm.texi: Likewise.
>> > 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
>> > 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
>> > 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
>> > 	strcmp for comparisons of asmspec with a register name if 
>> > 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.  
>> 
>> I really don't think we should be adding new target macros for things
>> like this.  The code is hardly on the critical path, so I don't think
>> compile time is a concern.  That said...
>> 
>> ...
>> 
>> So TBH I still prefer the DEFHOOKPOD suggestion.  I won't object if
>> someone else wants to approve the macro version though.
>
> Ok, as you say, this code isn't on the critical path so I'd be happy to change
> this to a DEFHOOKPOD.
>
> In general, what should be considered when deciding between a hook and macro?
> Does the choice lean towards macros mainly when compile time is a concern? And
> hooks otherwise?
>
> Is the downside of this macro implementation compared to a DEFHOOKPOD mainly
> just the maintainability/readability of the added code?

Macros are essentially the "old way" and target hooks the "new way".
The decision was made a long time ago to move away from macros where
possible.  TBH I no longer remember the reasons clearly, but I think
it was partly to avoid including so much target stuff in non-target
files, and partly in the hope that we might one day support multiple
targets in a single build.  Years later, we're not much closer to the
latter and I'm not sure it's a realistic goal.

So over time various macros have been moved to hooks and new target
tests should generally use hooks if at all possible.

That said, there are some macros that are too compile-time sensitive
to be hooks or variables.  BITS_PER_UNIT is the most obvious example.
Also, the current approach of using header files to control OS and
object format features makes it difficult to convert the related
macros to hooks in a natural way.

Thanks,
Richard

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Jozef Lawrynowicz]

On Fri, 19 Jul 2019 10:55:59 +0100
Richard Sandiford <richard.sandiford@arm.com> wrote:

> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> > On Fri, 19 Jul 2019 09:31:15 +0100
> > Richard Sandiford <richard.sandiford@arm.com> wrote:
> >  
> >> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:  
> >> >
> >> > From 82eadcdcbb8914b06818f7c8a10156336518e8d1 Mon Sep 17 00:00:00 2001
> >> > From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
> >> > Date: Wed, 17 Jul 2019 11:48:23 +0100
> >> > Subject: [PATCH] Implement CASE_INSENSITIVE_REGISTER_NAMES
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> > 2019-07-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
> >> >
> >> > 	PR target/70320
> >> > 	* doc/tm.texi.in: Document new macro CASE_INSENSITIVE_REGISTER_NAMES.
> >> > 	* doc/tm.texi: Likewise.
> >> > 	* defaults.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 0.
> >> > 	* config/msp430/msp430.h: Define CASE_INSENSITIVE_REGISTER_NAMES to 1.
> >> > 	* varasm.c (decode_reg_name_and_count): Use strcasecmp instead of
> >> > 	strcmp for comparisons of asmspec with a register name if 
> >> > 	CASE_INSENSITIVE_REGISTER_NAMES is defined to 1.    
> >> 
> >> I really don't think we should be adding new target macros for things
> >> like this.  The code is hardly on the critical path, so I don't think
> >> compile time is a concern.  That said...
> >> 
> >> ...
> >> 
> >> So TBH I still prefer the DEFHOOKPOD suggestion.  I won't object if
> >> someone else wants to approve the macro version though.  
> >
> > Ok, as you say, this code isn't on the critical path so I'd be happy to change
> > this to a DEFHOOKPOD.
> >
> > In general, what should be considered when deciding between a hook and macro?
> > Does the choice lean towards macros mainly when compile time is a concern? And
> > hooks otherwise?
> >
> > Is the downside of this macro implementation compared to a DEFHOOKPOD mainly
> > just the maintainability/readability of the added code?  
> 
> Macros are essentially the "old way" and target hooks the "new way".
> The decision was made a long time ago to move away from macros where
> possible.  TBH I no longer remember the reasons clearly, but I think
> it was partly to avoid including so much target stuff in non-target
> files, and partly in the hope that we might one day support multiple
> targets in a single build.  Years later, we're not much closer to the
> latter and I'm not sure it's a realistic goal.
> 
> So over time various macros have been moved to hooks and new target
> tests should generally use hooks if at all possible.
> 
> That said, there are some macros that are too compile-time sensitive
> to be hooks or variables.  BITS_PER_UNIT is the most obvious example.
> Also, the current approach of using header files to control OS and
> object format features makes it difficult to convert the related
> macros to hooks in a natural way.

Ok great, thanks for the info, 

Jozef
> 
> Thanks,
> Richard

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Segher Boessenkool]

On Fri, Jul 19, 2019 at 10:55:59AM +0100, Richard Sandiford wrote:
> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> > Is the downside of this macro implementation compared to a DEFHOOKPOD mainly
> > just the maintainability/readability of the added code?
> 
> Macros are essentially the "old way" and target hooks the "new way".
> The decision was made a long time ago to move away from macros where
> possible.  TBH I no longer remember the reasons clearly, but I think
> it was partly to avoid including so much target stuff in non-target
> files, and partly in the hope that we might one day support multiple
> targets in a single build.  Years later, we're not much closer to the
> latter and I'm not sure it's a realistic goal.

Macros also are hard to use: they do not follow the normal language rules,
they just do text replacement (and C macros are terribly weak, at that).
Some of this is made better by conventions like "always put all uses of
macro parameters in parentheses", but that itself is an extra mental load
to remember.

> That said, there are some macros that are too compile-time sensitive
> to be hooks or variables.  BITS_PER_UNIT is the most obvious example.

Maybe C++ can help here?  Or maybe we can declare some hooks as inline
functions somehow?


Segher

Re: [PATCH] Allow case-insensitive comparisons of register names by implementing CASE_INSENSITIVE_REGISTER_NAMES PR target/70320

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Fri, Jul 19, 2019 at 10:55:59AM +0100, Richard Sandiford wrote:
>> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
>> > Is the downside of this macro implementation compared to a DEFHOOKPOD mainly
>> > just the maintainability/readability of the added code?
>> 
>> Macros are essentially the "old way" and target hooks the "new way".
>> The decision was made a long time ago to move away from macros where
>> possible.  TBH I no longer remember the reasons clearly, but I think
>> it was partly to avoid including so much target stuff in non-target
>> files, and partly in the hope that we might one day support multiple
>> targets in a single build.  Years later, we're not much closer to the
>> latter and I'm not sure it's a realistic goal.
>
> Macros also are hard to use: they do not follow the normal language rules,
> they just do text replacement (and C macros are terribly weak, at that).
> Some of this is made better by conventions like "always put all uses of
> macro parameters in parentheses", but that itself is an extra mental load
> to remember.
>
>> That said, there are some macros that are too compile-time sensitive
>> to be hooks or variables.  BITS_PER_UNIT is the most obvious example.
>
> Maybe C++ can help here?  Or maybe we can declare some hooks as inline
> functions somehow?

FWIW, I have a WIP patch that uses C++ here.  I expect people will
hate it :-)

Richard