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

This patch may have been lost in the Great LMO Firewall Saga, so I am resending it:

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

Quoted 12 lines · David Daney

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

include/asm-mips/bitops.h static inline void set_bit(unsigned long nr, volatile unsigned long *addr) · line 88

Quoted 8 lines

include/asm-mips/bitops.h +1 -1

@@ -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?

Quoted 30 lines · Ralf Baechle

Quoted 12 lines · David Daney

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

include/asm-mips/bitops.h static inline void set_bit(unsigned long nr, volatile unsigned long *addr) · line 88

Quoted 8 lines

include/asm-mips/bitops.h +1 -1

@@ -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.

Quoted 3 lines

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.

Quoted 30 lines · David Daney

Quoted 29 lines · Ralf Baechle

Quoted 12 lines · David Daney

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

include/asm-mips/bitops.h static inline void set_bit(unsigned long nr, volatile unsigned long *addr) · line 88

Quoted 8 lines

include/asm-mips/bitops.h +1 -1

@@ -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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 46 lines · Richard Sandiford

Quoted 30 lines · David Daney

Quoted 29 lines · Ralf Baechle

Quoted 12 lines · David Daney

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

include/asm-mips/bitops.h static inline void set_bit(unsigned long nr, volatile unsigned long *addr) · line 88

Quoted 8 lines

include/asm-mips/bitops.h +1 -1

@@ -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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

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?

Quoted 74 lines · David Daney

Quoted 45 lines · Richard Sandiford

Quoted 30 lines · David Daney

Quoted 29 lines · Ralf Baechle

Quoted 12 lines · David Daney

The third operand to 'ins' must be a constant int, not a register.

Signed-off-by David Daney <[email protected]>

---
include/asm-mips/bitops.h | 6 +++---

1 files changed, 3 insertions(+), 3 deletions(-)

include/asm-mips/bitops.h +0 -0

index 6427247..9a7274b 100644

include/asm-mips/bitops.h static inline void set_bit(unsigned long nr, volatile unsigned long *addr) · line 88

Quoted 8 lines

include/asm-mips/bitops.h +1 -1

@@ -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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 5 lines

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);
.
.
.

Quoted 3 lines

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.)

Quoted 9 lines

Quoted 3 lines

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:

Quoted 9 lines

    __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.