* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. [not found] ` <20080611172950.GA16600@linux-mips.org> @ 2008-06-11 17:44 ` David Daney 2008-06-12 8:27 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: David Daney @ 2008-06-11 17:44 UTC (permalink / raw) To: Ralf Baechle, GCC Mailing List; +Cc: MIPS Linux List, Richard Sandiford Ralf Baechle wrote: > On Wed, Jun 11, 2008 at 10:04:25AM -0700, David Daney wrote: > >> The third operand to 'ins' must be a constant int, not a register. >> >> Signed-off-by: David Daney <ddaney@avtrex.com> >> --- >> include/asm-mips/bitops.h | 6 +++--- >> 1 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h >> index 6427247..9a7274b 100644 >> --- a/include/asm-mips/bitops.h >> +++ b/include/asm-mips/bitops.h >> @@ -82,7 +82,7 @@ static inline void set_bit(unsigned long nr, volatile unsigned long *addr) >> "2: b 1b \n" >> " .previous \n" >> : "=&r" (temp), "=m" (*m) >> - : "ir" (bit), "m" (*m), "r" (~0)); >> + : "i" (bit), "m" (*m), "r" (~0)); >> #endif /* CONFIG_CPU_MIPSR2 */ >> } else if (cpu_has_llsc) { >> __asm__ __volatile__( > > An old trick to get gcc to do the right thing. Basically at the stage when > gcc is verifying the constraints it may not yet know that it can optimize > things into an "i" argument, so compilation may fail if "r" isn't in the > constraints. However we happen to know that due to the way the code is > written gcc will always be able to make use of the "i" constraint so no > code using "r" should ever be created. > > The trick is a bit ugly; I think it was used first in asm-i386/io.h ages ago > and I would be happy if we could get rid of it without creating new problems. > Maybe a gcc hacker here can tell more? It is not nice to lie to GCC. CCing GCC and Richard in hopes that a wider audience may shed some light on the issue. David Daney ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. 2008-06-11 17:44 ` Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions David Daney @ 2008-06-12 8:27 ` Richard Sandiford 2008-06-12 16:31 ` David Daney 0 siblings, 1 reply; 6+ messages in thread From: Richard Sandiford @ 2008-06-12 8:27 UTC (permalink / raw) To: David Daney; +Cc: Ralf Baechle, GCC Mailing List, MIPS Linux List David Daney <ddaney@avtrex.com> writes: > Ralf Baechle wrote: >> On Wed, Jun 11, 2008 at 10:04:25AM -0700, David Daney wrote: >> >>> The third operand to 'ins' must be a constant int, not a register. >>> >>> Signed-off-by: David Daney <ddaney@avtrex.com> >>> --- >>> include/asm-mips/bitops.h | 6 +++--- >>> 1 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h >>> index 6427247..9a7274b 100644 >>> --- a/include/asm-mips/bitops.h >>> +++ b/include/asm-mips/bitops.h >>> @@ -82,7 +82,7 @@ static inline void set_bit(unsigned long nr, volatile unsigned long *addr) >>> "2: b 1b \n" >>> " .previous \n" >>> : "=&r" (temp), "=m" (*m) >>> - : "ir" (bit), "m" (*m), "r" (~0)); >>> + : "i" (bit), "m" (*m), "r" (~0)); >>> #endif /* CONFIG_CPU_MIPSR2 */ >>> } else if (cpu_has_llsc) { >>> __asm__ __volatile__( >> >> An old trick to get gcc to do the right thing. Basically at the stage when >> gcc is verifying the constraints it may not yet know that it can optimize >> things into an "i" argument, so compilation may fail if "r" isn't in the >> constraints. However we happen to know that due to the way the code is >> written gcc will always be able to make use of the "i" constraint so no >> code using "r" should ever be created. >> >> The trick is a bit ugly; I think it was used first in asm-i386/io.h ages ago >> and I would be happy if we could get rid of it without creating new problems. >> Maybe a gcc hacker here can tell more? > > It is not nice to lie to GCC. > > CCing GCC and Richard in hopes that a wider audience may shed some light on the issue. You _might_ be able to use "i#r" instead of "ri", but I wouldn't really recommend it. Even if it works now, I don't think there's any guarantee it will in future. There are tricks you could pull to detect the problem at compile time rather than assembly time, but that's probably not a big win. And again, I wouldn't recommend them. I'm not saying anything you don't know here, but if the argument is always a syntactic constant, the safest bet would be to apply David's patch and also convert the function into a macro. I notice some other ports use macros rather than inline functions here. I assume you've deliberately rejected macros as being too ugly though. Richard ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. 2008-06-12 8:27 ` Richard Sandiford @ 2008-06-12 16:31 ` David Daney 2008-06-12 18:22 ` Richard Sandiford 0 siblings, 1 reply; 6+ messages in thread From: David Daney @ 2008-06-12 16:31 UTC (permalink / raw) To: Ralf Baechle, GCC Mailing List, MIPS Linux List, rdsandiford Richard Sandiford wrote: > David Daney <ddaney@avtrex.com> writes: >> Ralf Baechle wrote: >>> On Wed, Jun 11, 2008 at 10:04:25AM -0700, David Daney wrote: >>> >>>> The third operand to 'ins' must be a constant int, not a register. >>>> >>>> Signed-off-by: David Daney <ddaney@avtrex.com> >>>> --- >>>> include/asm-mips/bitops.h | 6 +++--- >>>> 1 files changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h >>>> index 6427247..9a7274b 100644 >>>> --- a/include/asm-mips/bitops.h >>>> +++ b/include/asm-mips/bitops.h >>>> @@ -82,7 +82,7 @@ static inline void set_bit(unsigned long nr, volatile unsigned long *addr) >>>> "2: b 1b \n" >>>> " .previous \n" >>>> : "=&r" (temp), "=m" (*m) >>>> - : "ir" (bit), "m" (*m), "r" (~0)); >>>> + : "i" (bit), "m" (*m), "r" (~0)); >>>> #endif /* CONFIG_CPU_MIPSR2 */ >>>> } else if (cpu_has_llsc) { >>>> __asm__ __volatile__( >>> An old trick to get gcc to do the right thing. Basically at the stage when >>> gcc is verifying the constraints it may not yet know that it can optimize >>> things into an "i" argument, so compilation may fail if "r" isn't in the >>> constraints. However we happen to know that due to the way the code is >>> written gcc will always be able to make use of the "i" constraint so no >>> code using "r" should ever be created. >>> >>> The trick is a bit ugly; I think it was used first in asm-i386/io.h ages ago >>> and I would be happy if we could get rid of it without creating new problems. >>> Maybe a gcc hacker here can tell more? >> It is not nice to lie to GCC. >> >> CCing GCC and Richard in hopes that a wider audience may shed some light on the issue. > > You _might_ be able to use "i#r" instead of "ri", but I wouldn't > really recommend it. Even if it works now, I don't think there's > any guarantee it will in future. > > There are tricks you could pull to detect the problem at compile time > rather than assembly time, but that's probably not a big win. And again, > I wouldn't recommend them. > > I'm not saying anything you don't know here, but if the argument is > always a syntactic constant, the safest bet would be to apply David's > patch and also convert the function into a macro. I notice some other > ports use macros rather than inline functions here. I assume you've > deliberately rejected macros as being too ugly though. I am still a little unclear on this. To restate the question: static inline void f(unsigned nr, unsigned *p) { unsigned short bit = nr & 5; if (__builtin_constant_p(bit)) { __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (bit)); } else { // Do something else. } } . . . f(3, some_pointer); . . . Among the versions of GCC that can build the current kernel, will any fail on this code because the "i" constraint cannot be matched when expanded to RTL? David Daney ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. 2008-06-12 16:31 ` David Daney @ 2008-06-12 18:22 ` Richard Sandiford 2008-06-12 20:10 ` David Daney 0 siblings, 1 reply; 6+ messages in thread From: Richard Sandiford @ 2008-06-12 18:22 UTC (permalink / raw) To: David Daney; +Cc: Ralf Baechle, GCC Mailing List, MIPS Linux List David Daney <ddaney@avtrex.com> writes: > Richard Sandiford wrote: >> David Daney <ddaney@avtrex.com> writes: >>> Ralf Baechle wrote: >>>> On Wed, Jun 11, 2008 at 10:04:25AM -0700, David Daney wrote: >>>> >>>>> The third operand to 'ins' must be a constant int, not a register. >>>>> >>>>> Signed-off-by: David Daney <ddaney@avtrex.com> >>>>> --- >>>>> include/asm-mips/bitops.h | 6 +++--- >>>>> 1 files changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h >>>>> index 6427247..9a7274b 100644 >>>>> --- a/include/asm-mips/bitops.h >>>>> +++ b/include/asm-mips/bitops.h >>>>> @@ -82,7 +82,7 @@ static inline void set_bit(unsigned long nr, volatile unsigned long *addr) >>>>> "2: b 1b \n" >>>>> " .previous \n" >>>>> : "=&r" (temp), "=m" (*m) >>>>> - : "ir" (bit), "m" (*m), "r" (~0)); >>>>> + : "i" (bit), "m" (*m), "r" (~0)); >>>>> #endif /* CONFIG_CPU_MIPSR2 */ >>>>> } else if (cpu_has_llsc) { >>>>> __asm__ __volatile__( >>>> An old trick to get gcc to do the right thing. Basically at the stage when >>>> gcc is verifying the constraints it may not yet know that it can optimize >>>> things into an "i" argument, so compilation may fail if "r" isn't in the >>>> constraints. However we happen to know that due to the way the code is >>>> written gcc will always be able to make use of the "i" constraint so no >>>> code using "r" should ever be created. >>>> >>>> The trick is a bit ugly; I think it was used first in asm-i386/io.h ages ago >>>> and I would be happy if we could get rid of it without creating new problems. >>>> Maybe a gcc hacker here can tell more? >>> It is not nice to lie to GCC. >>> >>> CCing GCC and Richard in hopes that a wider audience may shed some light on the issue. >> >> You _might_ be able to use "i#r" instead of "ri", but I wouldn't >> really recommend it. Even if it works now, I don't think there's >> any guarantee it will in future. >> >> There are tricks you could pull to detect the problem at compile time >> rather than assembly time, but that's probably not a big win. And again, >> I wouldn't recommend them. >> >> I'm not saying anything you don't know here, but if the argument is >> always a syntactic constant, the safest bet would be to apply David's >> patch and also convert the function into a macro. I notice some other >> ports use macros rather than inline functions here. I assume you've >> deliberately rejected macros as being too ugly though. > > I am still a little unclear on this. > > To restate the question: > > static inline void f(unsigned nr, unsigned *p) > { > unsigned short bit = nr & 5; > > if (__builtin_constant_p(bit)) { > __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (bit)); > } > else { > // Do something else. > } > } > . > . > . > f(3, some_pointer); > . > . > . > > Among the versions of GCC that can build the current kernel, will any > fail on this code because the "i" constraint cannot be matched when > expanded to RTL? Someone will point this out if I don't, so for avoidance of doubt: this needs to be always_inline. It also isn't guaranteed to work with "bit" being a separate statement. I'm not truly sure it's guaranteed to work even with: __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (nr & 5)); but I think we'd try hard to make sure it does. I think Maciej said that 3.2 was the minimum current version. Even with those two issues sorted out, I don't think you can rely on this sort of thing with compilers that used RTL inlining. (always_inline does go back to 3.2, in case you're wondering.) Richard ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. 2008-06-12 18:22 ` Richard Sandiford @ 2008-06-12 20:10 ` David Daney 2008-06-13 14:29 ` Ralf Baechle 0 siblings, 1 reply; 6+ messages in thread From: David Daney @ 2008-06-12 20:10 UTC (permalink / raw) To: David Daney, Ralf Baechle, GCC Mailing List, MIPS Linux List, rdsandiford Richard Sandiford wrote: > David Daney <ddaney@avtrex.com> writes: >> Richard Sandiford wrote: >>> David Daney <ddaney@avtrex.com> writes: >>>> Ralf Baechle wrote: >>>>> On Wed, Jun 11, 2008 at 10:04:25AM -0700, David Daney wrote: >>>>> >>>>>> The third operand to 'ins' must be a constant int, not a register. >>>>>> >>>>>> Signed-off-by: David Daney <ddaney@avtrex.com> >>>>>> --- >>>>>> include/asm-mips/bitops.h | 6 +++--- >>>>>> 1 files changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h >>>>>> index 6427247..9a7274b 100644 >>>>>> --- a/include/asm-mips/bitops.h >>>>>> +++ b/include/asm-mips/bitops.h >>>>>> @@ -82,7 +82,7 @@ static inline void set_bit(unsigned long nr, volatile unsigned long *addr) >>>>>> "2: b 1b \n" >>>>>> " .previous \n" >>>>>> : "=&r" (temp), "=m" (*m) >>>>>> - : "ir" (bit), "m" (*m), "r" (~0)); >>>>>> + : "i" (bit), "m" (*m), "r" (~0)); >>>>>> #endif /* CONFIG_CPU_MIPSR2 */ >>>>>> } else if (cpu_has_llsc) { >>>>>> __asm__ __volatile__( >>>>> An old trick to get gcc to do the right thing. Basically at the stage when >>>>> gcc is verifying the constraints it may not yet know that it can optimize >>>>> things into an "i" argument, so compilation may fail if "r" isn't in the >>>>> constraints. However we happen to know that due to the way the code is >>>>> written gcc will always be able to make use of the "i" constraint so no >>>>> code using "r" should ever be created. >>>>> >>>>> The trick is a bit ugly; I think it was used first in asm-i386/io.h ages ago >>>>> and I would be happy if we could get rid of it without creating new problems. >>>>> Maybe a gcc hacker here can tell more? >>>> It is not nice to lie to GCC. >>>> >>>> CCing GCC and Richard in hopes that a wider audience may shed some light on the issue. >>> You _might_ be able to use "i#r" instead of "ri", but I wouldn't >>> really recommend it. Even if it works now, I don't think there's >>> any guarantee it will in future. >>> >>> There are tricks you could pull to detect the problem at compile time >>> rather than assembly time, but that's probably not a big win. And again, >>> I wouldn't recommend them. >>> >>> I'm not saying anything you don't know here, but if the argument is >>> always a syntactic constant, the safest bet would be to apply David's >>> patch and also convert the function into a macro. I notice some other >>> ports use macros rather than inline functions here. I assume you've >>> deliberately rejected macros as being too ugly though. >> I am still a little unclear on this. >> >> To restate the question: >> >> static inline void f(unsigned nr, unsigned *p) >> { >> unsigned short bit = nr & 5; >> >> if (__builtin_constant_p(bit)) { >> __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (bit)); >> } >> else { >> // Do something else. >> } >> } >> . >> . >> . >> f(3, some_pointer); >> . >> . >> . >> >> Among the versions of GCC that can build the current kernel, will any >> fail on this code because the "i" constraint cannot be matched when >> expanded to RTL? > > Someone will point this out if I don't, so for avoidance of doubt: > this needs to be always_inline. It also isn't guaranteed to work > with "bit" being a separate statement. I'm not truly sure it's > guaranteed to work even with: > > __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (nr & 5)); > > but I think we'd try hard to make sure it does. > > I think Maciej said that 3.2 was the minimum current version. > Even with those two issues sorted out, I don't think you can > rely on this sort of thing with compilers that used RTL inlining. > (always_inline does go back to 3.2, in case you're wondering.) > Well I withdraw the patch. With the current kernel code we seem to always get good code generation. In the event that the compiler tries to put the shift amount (nr) in a register, the assembler will complain. I don't think it is possible to generate bad object code, so best to leave it alone. FYI, the reason that I stumbled on this several weeks ago is that if(__builtin_constant_p(nr)) in the trunk compiler was generating code for the asm even though nr was not constant. David Daney ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions. 2008-06-12 20:10 ` David Daney @ 2008-06-13 14:29 ` Ralf Baechle 0 siblings, 0 replies; 6+ messages in thread From: Ralf Baechle @ 2008-06-13 14:29 UTC (permalink / raw) To: David Daney; +Cc: GCC Mailing List, MIPS Linux List, rdsandiford On Thu, Jun 12, 2008 at 01:10:10PM -0700, David Daney wrote: >>> Among the versions of GCC that can build the current kernel, will any >>> fail on this code because the "i" constraint cannot be matched when >>> expanded to RTL? >> >> Someone will point this out if I don't, so for avoidance of doubt: >> this needs to be always_inline. It also isn't guaranteed to work >> with "bit" being a separate statement. I'm not truly sure it's >> guaranteed to work even with: >> >> __asm__ __volatile__ (" foo %0, %1" : "=m" (*p) : "i" (nr & 5)); >> >> but I think we'd try hard to make sure it does. >> >> I think Maciej said that 3.2 was the minimum current version. >> Even with those two issues sorted out, I don't think you can >> rely on this sort of thing with compilers that used RTL inlining. >> (always_inline does go back to 3.2, in case you're wondering.) >> > > Well I withdraw the patch. With the current kernel code we seem to always get good code generation. In the event that the compiler tries to put the shift amount (nr) in a register, the assembler will complain. I don't think it is possible to generate bad object code, so best to leave it alone. > > FYI, the reason that I stumbled on this several weeks ago is that if(__builtin_constant_p(nr)) in the trunk compiler was generating code for the asm even though nr was not constant. How about I simply put your patch into the -queue tree, everybody gives it a nice beating and then we'll how well it'll hold up in the real world? Ralf ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2008-06-13 14:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <48500599.9080807@avtrex.com> [not found] ` <20080611172950.GA16600@linux-mips.org> 2008-06-11 17:44 ` Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions David Daney 2008-06-12 8:27 ` Richard Sandiford 2008-06-12 16:31 ` David Daney 2008-06-12 18:22 ` Richard Sandiford 2008-06-12 20:10 ` David Daney 2008-06-13 14:29 ` Ralf Baechle
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).