diff mbox series

[powerpc] Tighten contraints for asm constant parameters

Message ID 20211019151413.123039-1-pc@us.ibm.com
State Committed
Commit 9fea0f1a2a6e4f7946505c358ab99ea3ab846274
Delegated to: Tulio Magno Quites Machado Filho
Headers show
Series [powerpc] Tighten contraints for asm constant parameters | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Paul A. Clarke Oct. 19, 2021, 3:14 p.m. UTC
There are a few places where only constants are acceptable for `asm`
parameters, yet the constraint "i" is used.  "i" is for "any integer"
including variables.

Use "n" instead of "i" where constant integers are required.

Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>
---
 sysdeps/powerpc/fpu/fenv_libc.h  | 8 ++++----
 sysdeps/powerpc/test-get_hwcap.c | 8 ++++----
 sysdeps/powerpc/tst-tlsifunc.c   | 2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

Comments

Florian Weimer Oct. 19, 2021, 3:26 p.m. UTC | #1
* Paul A. Clarke via Libc-alpha:

> There are a few places where only constants are acceptable for `asm`
> parameters, yet the constraint "i" is used.  "i" is for "any integer"
> including variables.
>
> Use "n" instead of "i" where constant integers are required.

This doesn't seem to match the GCC documentation:

| 'i'
|      An immediate integer operand (one with constant value) is allowed.
|      This includes symbolic constants whose values will be known only at
|      assembly time or later.

And the RS6000 documentation does not override that.

Thanks,
Florian
Paul A. Clarke Oct. 19, 2021, 3:58 p.m. UTC | #2
On Tue, Oct 19, 2021 at 05:26:36PM +0200, Florian Weimer wrote:
> * Paul A. Clarke via Libc-alpha:
> 
> > There are a few places where only constants are acceptable for `asm`
> > parameters, yet the constraint "i" is used.  "i" is for "any integer"
> > including variables.
> >
> > Use "n" instead of "i" where constant integers are required.
> 
> This doesn't seem to match the GCC documentation:
> 
> | 'i'
> |      An immediate integer operand (one with constant value) is allowed.
> |      This includes symbolic constants whose values will be known only at
> |      assembly time or later.
> 
> And the RS6000 documentation does not override that.

I was too loose with my characterization of 'i', in contrast to 'n':

| ‘n’
|      An immediate integer operand with a known numeric value is allowed.
|      Many systems cannot support assembly-time constants for operands less
|      than a word wide. Constraints for these operands should use ‘n’
|      rather than ‘i’.

The cases changed by the patch require a *known numeric value*, as they are
used as immediate values (hardcoded in the generated opcode).

I will reword to:
  There are a few places where only known numeric values are acceptable for
  `asm` parameters, yet the constraint "i" is used.  "i" can include
  "symbolic constants whose values will be known only at assembly time or
  later."

  Use "n" instead of "i" where known numeric values are required.

Does that work better?

PC
Florian Weimer Oct. 19, 2021, 3:59 p.m. UTC | #3
* Paul A. Clarke:

> I was too loose with my characterization of 'i', in contrast to 'n':
>
> | ‘n’
> |      An immediate integer operand with a known numeric value is allowed.
> |      Many systems cannot support assembly-time constants for operands less
> |      than a word wide. Constraints for these operands should use ‘n’
> |      rather than ‘i’.
>
> The cases changed by the patch require a *known numeric value*, as they are
> used as immediate values (hardcoded in the generated opcode).
>
> I will reword to:
>   There are a few places where only known numeric values are acceptable for
>   `asm` parameters, yet the constraint "i" is used.  "i" can include
>   "symbolic constants whose values will be known only at assembly time or
>   later."
>
>   Use "n" instead of "i" where known numeric values are required.
>
> Does that work better?

Yes, that explains the difference.  Thanks!

Florian
Tulio Magno Quites Machado Filho Oct. 29, 2021, 7:26 p.m. UTC | #4
"Paul A. Clarke" <pc@us.ibm.com> writes:

> There are a few places where only constants are acceptable for `asm`
> parameters, yet the constraint "i" is used.  "i" is for "any integer"
> including variables.
>
> Use "n" instead of "i" where constant integers are required.
>
> Suggested-by: Segher Boessenkool <segher@kernel.crashing.org>

LGTM.

Reviewed-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
diff mbox series

Patch

diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
index dc35b9dbe0d0..a04fb928cae2 100644
--- a/sysdeps/powerpc/fpu/fenv_libc.h
+++ b/sysdeps/powerpc/fpu/fenv_libc.h
@@ -73,7 +73,7 @@  extern const fenv_t *__fe_mask_env (void) attribute_hidden;
     if (__builtin_constant_p (rn))					\
       __asm__ __volatile__ (						\
         ".machine push; .machine \"power9\"; mffscrni %0,%1; .machine pop" \
-        : "=f" (__fr.fenv) : "i" (rn));					\
+        : "=f" (__fr.fenv) : "n" (rn));					\
     else								\
     {									\
       __fr.l = (rn);							\
@@ -135,8 +135,8 @@  extern const fenv_t *__fe_mask_env (void) attribute_hidden;
 /* Set/clear a particular FPSCR bit (for instance,
    reset_fpscr_bit(FPSCR_VE);
    prevents INVALID exceptions from being raised).  */
-#define set_fpscr_bit(x) asm volatile ("mtfsb1 %0" : : "i"(x))
-#define reset_fpscr_bit(x) asm volatile ("mtfsb0 %0" : : "i"(x))
+#define set_fpscr_bit(x) asm volatile ("mtfsb1 %0" : : "n"(x))
+#define reset_fpscr_bit(x) asm volatile ("mtfsb0 %0" : : "n"(x))
 
 typedef union
 {
@@ -184,7 +184,7 @@  __fesetround_inline_nocheck (const int round)
   if (__glibc_likely (GLRO(dl_hwcap2) & PPC_FEATURE2_ARCH_3_00))
     __fe_mffscrn (round);
   else
-    asm volatile ("mtfsfi 7,%0" : : "i" (round));
+    asm volatile ("mtfsfi 7,%0" : : "n" (round));
 #endif
 }
 
diff --git a/sysdeps/powerpc/test-get_hwcap.c b/sysdeps/powerpc/test-get_hwcap.c
index b5cef93cddd4..a64b63080756 100644
--- a/sysdeps/powerpc/test-get_hwcap.c
+++ b/sysdeps/powerpc/test-get_hwcap.c
@@ -63,16 +63,16 @@  uint64_t check_tcbhwcap (long tid)
 #ifdef __powerpc64__
   __asm__  ("ld %0,%1(%2)\n"
 	    : "=r" (tcb_hwcap)
-	    : "i" (__HWCAPOFF), "b" (__tp));
+	    : "n" (__HWCAPOFF), "b" (__tp));
 #else
   uint64_t h1, h2;
 
   __asm__ ("lwz %0,%1(%2)\n"
       : "=r" (h1)
-      : "i" (__HWCAPOFF), "b" (__tp));
+      : "n" (__HWCAPOFF), "b" (__tp));
   __asm__ ("lwz %0,%1(%2)\n"
       : "=r" (h2)
-      : "i" (__HWCAP2OFF), "b" (__tp));
+      : "n" (__HWCAP2OFF), "b" (__tp));
   tcb_hwcap = (h1 >> 32) << 32 | (h2 >> 32);
 #endif
 
@@ -117,7 +117,7 @@  uint64_t check_tcbhwcap (long tid)
   /* Same test for the platform number.  */
   __asm__  ("lwz %0,%1(%2)\n"
 	    : "=r" (tcb_at_platform)
-	    : "i" (__ATPLATOFF), "b" (__tp));
+	    : "n" (__ATPLATOFF), "b" (__tp));
 
   at_platform_string = (const char *) getauxval (AT_PLATFORM);
   at_platform = _dl_string_platform (at_platform_string);
diff --git a/sysdeps/powerpc/tst-tlsifunc.c b/sysdeps/powerpc/tst-tlsifunc.c
index c8c0bada4547..f2eaf11bb407 100644
--- a/sysdeps/powerpc/tst-tlsifunc.c
+++ b/sysdeps/powerpc/tst-tlsifunc.c
@@ -49,7 +49,7 @@  get_platform (void)
 
   __asm__  ("lwz %0,%1(%2)\n"
 	    : "=r" (tmp)
-	    : "i" (__ATPLATOFF), "b" (tp));
+	    : "n" (__ATPLATOFF), "b" (tp));
 
   return tmp;
 }