Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)
On Wed, Jul 10, 2019 at 05:23:28PM +0100, Jozef Lawrynowicz wrote: > On Tue, 9 Jul 2019 16:36:46 -0500 > Segher Boessenkool 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
Re: [PATCH] Perform case-insensitive comparison when decoding register names (PR target/70320)
On Tue, 9 Jul 2019 16:36:46 -0500
Segher Boessenkool 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 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)
On Tue, Jul 09, 2019 at 10:16:31PM +0100, Jozef Lawrynowicz wrote: > On Mon, 8 Jul 2019 16:42:15 -0500 > Segher Boessenkool 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)
On Mon, 8 Jul 2019 16:42:15 -0500 Segher Boessenkool 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)
On Mon, Jul 08, 2019 at 10:21:29PM +0100, Jozef Lawrynowicz wrote:
> On Mon, 08 Jul 2019 21:14:36 +0100
> Richard Sandiford wrote:
>
> > Segher Boessenkool 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)
On Mon, 08 Jul 2019 21:14:36 +0100 Richard Sandiford wrote: > Segher Boessenkool 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 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)
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
