Re: Resend: [PATCH] [MIPS] Fix asm constraints for 'ins' instructions.

[David Daney <ddaney@avtrex.com>]

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