diff mbox series

x86: Define __rdtsc and __rdtscp as macros

Message ID CAMe9rOpJbem4tnkdDX0NwpyMMLLzcakaij120aNCDXsW=wjRww@mail.gmail.com
State New
Headers show
Series x86: Define __rdtsc and __rdtscp as macros | expand

Commit Message

H.J. Lu March 26, 2021, 5:24 p.m. UTC 
On Fri, Mar 26, 2021 at 5:09 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 11:26 AM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 11:13:21AM +0100, Richard Biener wrote:
> > > On Fri, Mar 26, 2021 at 9:34 AM Jakub Jelinek via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > >
> > > > On Thu, Mar 25, 2021 at 11:36:37AM -0700, H.J. Lu via Gcc-patches wrote:
> > > > > How can we move forward with it?  I'd like to resolve it in GCC 11.
> > > >
> > > > I think it is too late for GCC 11 for this.
> > > > Especially if the solution would be that we change the behavior of existing
> > > > attribute, we would need enough time to test everything in the wild that
> > > > we don't break it badly,
> > >
> > > But isn't the suggested change only going to make programs we reject now
> > > with an error accepted or ICEing?  Thus, no program that works right now
> > > should break.
> >
> > That is true, but even
> > accepts-invalid
> > and
> > ice-on-invalid-code
> > would be important regressions.
> > Changing the always_inline attribute behavior without at least avoiding
> > the first of those for our intrinsics would be bad, and we need to look what
> > people use always_inline in the wild for and what are their expectations.
> > And for the intrinsics we need something maintainable, we have > 5000
> > intrinsics on i386 alone, > 4000 on aarch64, > 7000 on arm, > 600 on rs6000,
> > > 100 on sparc, I bet most of them rely on the current behavior.
> > I think the world doesn't end if we do it for GCC 12 only, do it right for
> > everything we are aware of and have many months to figure out what impact it
> > will have on programs in the wild.
>
> As said, my opinion is that this fallout doesn't "exist" in the wild
> since it can
> only exist for code we reject right now which in my definition of
> "out in the wild" makes it not exist.  I consider only code accepted by
> the compiler as valid "out in the wild" example.
>
> See also the behavior of always-inline with regard to the optimize attribute.
>
> So yes, a better solution would be nice but I can't see any since the
> underlying issue is known since a long time and thus the pragmatic
> solution is the best (IMHO), also from a QOI perspective.  For intrinsics
> it also avoids differences with -O0 vs -O with what we accept and reject.

Here is a simple patch for GCC 11 by defining __rdtsc and __rdtscp
as macros.   OK for master?

Thanks.

Comments

Uros Bizjak March 30, 2021, 8:14 a.m. UTC | #1 
On Fri, Mar 26, 2021 at 6:24 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 5:09 AM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 11:26 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > >
> > > On Fri, Mar 26, 2021 at 11:13:21AM +0100, Richard Biener wrote:
> > > > On Fri, Mar 26, 2021 at 9:34 AM Jakub Jelinek via Gcc-patches
> > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > >
> > > > > On Thu, Mar 25, 2021 at 11:36:37AM -0700, H.J. Lu via Gcc-patches wrote:
> > > > > > How can we move forward with it?  I'd like to resolve it in GCC 11.
> > > > >
> > > > > I think it is too late for GCC 11 for this.
> > > > > Especially if the solution would be that we change the behavior of existing
> > > > > attribute, we would need enough time to test everything in the wild that
> > > > > we don't break it badly,
> > > >
> > > > But isn't the suggested change only going to make programs we reject now
> > > > with an error accepted or ICEing?  Thus, no program that works right now
> > > > should break.
> > >
> > > That is true, but even
> > > accepts-invalid
> > > and
> > > ice-on-invalid-code
> > > would be important regressions.
> > > Changing the always_inline attribute behavior without at least avoiding
> > > the first of those for our intrinsics would be bad, and we need to look what
> > > people use always_inline in the wild for and what are their expectations.
> > > And for the intrinsics we need something maintainable, we have > 5000
> > > intrinsics on i386 alone, > 4000 on aarch64, > 7000 on arm, > 600 on rs6000,
> > > > 100 on sparc, I bet most of them rely on the current behavior.
> > > I think the world doesn't end if we do it for GCC 12 only, do it right for
> > > everything we are aware of and have many months to figure out what impact it
> > > will have on programs in the wild.
> >
> > As said, my opinion is that this fallout doesn't "exist" in the wild
> > since it can
> > only exist for code we reject right now which in my definition of
> > "out in the wild" makes it not exist.  I consider only code accepted by
> > the compiler as valid "out in the wild" example.
> >
> > See also the behavior of always-inline with regard to the optimize attribute.
> >
> > So yes, a better solution would be nice but I can't see any since the
> > underlying issue is known since a long time and thus the pragmatic
> > solution is the best (IMHO), also from a QOI perspective.  For intrinsics
> > it also avoids differences with -O0 vs -O with what we accept and reject.
>
> Here is a simple patch for GCC 11 by defining __rdtsc and __rdtscp
> as macros.   OK for master?

I don't want to step on anyone's toes by approving this approach, so
I'd like to ask Richard and Jakub if they agree with the solution.

Uros,
Richard Biener March 30, 2021, 10:59 a.m. UTC | #2 
On Tue, Mar 30, 2021 at 10:14 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 6:24 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 5:09 AM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Fri, Mar 26, 2021 at 11:26 AM Jakub Jelinek <jakub@redhat.com> wrote:
> > > >
> > > > On Fri, Mar 26, 2021 at 11:13:21AM +0100, Richard Biener wrote:
> > > > > On Fri, Mar 26, 2021 at 9:34 AM Jakub Jelinek via Gcc-patches
> > > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > > >
> > > > > > On Thu, Mar 25, 2021 at 11:36:37AM -0700, H.J. Lu via Gcc-patches wrote:
> > > > > > > How can we move forward with it?  I'd like to resolve it in GCC 11.
> > > > > >
> > > > > > I think it is too late for GCC 11 for this.
> > > > > > Especially if the solution would be that we change the behavior of existing
> > > > > > attribute, we would need enough time to test everything in the wild that
> > > > > > we don't break it badly,
> > > > >
> > > > > But isn't the suggested change only going to make programs we reject now
> > > > > with an error accepted or ICEing?  Thus, no program that works right now
> > > > > should break.
> > > >
> > > > That is true, but even
> > > > accepts-invalid
> > > > and
> > > > ice-on-invalid-code
> > > > would be important regressions.
> > > > Changing the always_inline attribute behavior without at least avoiding
> > > > the first of those for our intrinsics would be bad, and we need to look what
> > > > people use always_inline in the wild for and what are their expectations.
> > > > And for the intrinsics we need something maintainable, we have > 5000
> > > > intrinsics on i386 alone, > 4000 on aarch64, > 7000 on arm, > 600 on rs6000,
> > > > > 100 on sparc, I bet most of them rely on the current behavior.
> > > > I think the world doesn't end if we do it for GCC 12 only, do it right for
> > > > everything we are aware of and have many months to figure out what impact it
> > > > will have on programs in the wild.
> > >
> > > As said, my opinion is that this fallout doesn't "exist" in the wild
> > > since it can
> > > only exist for code we reject right now which in my definition of
> > > "out in the wild" makes it not exist.  I consider only code accepted by
> > > the compiler as valid "out in the wild" example.
> > >
> > > See also the behavior of always-inline with regard to the optimize attribute.
> > >
> > > So yes, a better solution would be nice but I can't see any since the
> > > underlying issue is known since a long time and thus the pragmatic
> > > solution is the best (IMHO), also from a QOI perspective.  For intrinsics
> > > it also avoids differences with -O0 vs -O with what we accept and reject.
> >
> > Here is a simple patch for GCC 11 by defining __rdtsc and __rdtscp
> > as macros.   OK for master?
>
> I don't want to step on anyone's toes by approving this approach, so
> I'd like to ask Richard and Jakub if they agree with the solution.

I'm OK with the solution for __rdtsc & friends.  I suppose there's nothing
that guarantees taking the address of an intrinsic is going to work?

It of course still leaves the more general problem unsolved.

Richard.

> Uros,
Jakub Jelinek March 30, 2021, 11:03 a.m. UTC | #3 
On Tue, Mar 30, 2021 at 12:59:16PM +0200, Richard Biener wrote:
> > > > So yes, a better solution would be nice but I can't see any since the
> > > > underlying issue is known since a long time and thus the pragmatic
> > > > solution is the best (IMHO), also from a QOI perspective.  For intrinsics
> > > > it also avoids differences with -O0 vs -O with what we accept and reject.
> > >
> > > Here is a simple patch for GCC 11 by defining __rdtsc and __rdtscp
> > > as macros.   OK for master?
> >
> > I don't want to step on anyone's toes by approving this approach, so
> > I'd like to ask Richard and Jakub if they agree with the solution.
> 
> I'm OK with the solution for __rdtsc & friends.  

Ok for me too (temporarily until we have a fix for the general problem).

> I suppose there's nothing that guarantees taking the address of an intrinsic is going to work?

I bet one gets tons of different errors that way.  After all, for -O0 a lot
of intrinsics are macros.  And, for those that are inline functions, a lot
of them will be rejected if an immediate argument doesn't have a constant
value.

	Jakub
Uros Bizjak March 30, 2021, 11:40 a.m. UTC | #4 
On Tue, Mar 30, 2021 at 1:03 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Mar 30, 2021 at 12:59:16PM +0200, Richard Biener wrote:
> > > > > So yes, a better solution would be nice but I can't see any since the
> > > > > underlying issue is known since a long time and thus the pragmatic
> > > > > solution is the best (IMHO), also from a QOI perspective.  For intrinsics
> > > > > it also avoids differences with -O0 vs -O with what we accept and reject.
> > > >
> > > > Here is a simple patch for GCC 11 by defining __rdtsc and __rdtscp
> > > > as macros.   OK for master?
> > >
> > > I don't want to step on anyone's toes by approving this approach, so
> > > I'd like to ask Richard and Jakub if they agree with the solution.
> >
> > I'm OK with the solution for __rdtsc & friends.
>
> Ok for me too (temporarily until we have a fix for the general problem).
>
> > I suppose there's nothing that guarantees taking the address of an intrinsic is going to work?
>
> I bet one gets tons of different errors that way.  After all, for -O0 a lot
> of intrinsics are macros.  And, for those that are inline functions, a lot
> of them will be rejected if an immediate argument doesn't have a constant
> value.

LGTM for the patch.

Thanks,
Uros.
diff mbox series

Patch

From 84c0019ee2a7125daaad161bfbb98c3bf74ca48b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 23 Mar 2021 20:04:58 -0700
Subject: [PATCH] x86: Define __rdtsc and __rdtscp as macros

Define __rdtsc and __rdtscp as macros for callers with general-regs-only
target attribute to avoid inline failure with always_inline attribute.

gcc/

	PR target/99744
	* config/i386/ia32intrin.h (__rdtsc): Defined as macro.
	(__rdtscp): Likewise.

gcc/testsuite/

	PR target/99744
	* gcc.target/i386/pr99744-1.c: New test.
---
 gcc/config/i386/ia32intrin.h              | 14 ++-----------
 gcc/testsuite/gcc.target/i386/pr99744-1.c | 25 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr99744-1.c

diff --git a/gcc/config/i386/ia32intrin.h b/gcc/config/i386/ia32intrin.h
index d336a51669a..591394076cc 100644
--- a/gcc/config/i386/ia32intrin.h
+++ b/gcc/config/i386/ia32intrin.h
@@ -107,22 +107,12 @@  __rdpmc (int __S)
 #endif /* __iamcu__ */
 
 /* rdtsc */
-extern __inline unsigned long long
-__attribute__((__gnu_inline__, __always_inline__, __artificial__))
-__rdtsc (void)
-{
-  return __builtin_ia32_rdtsc ();
-}
+#define __rdtsc()		__builtin_ia32_rdtsc ()
 
 #ifndef __iamcu__
 
 /* rdtscp */
-extern __inline unsigned long long
-__attribute__((__gnu_inline__, __always_inline__, __artificial__))
-__rdtscp (unsigned int *__A)
-{
-  return __builtin_ia32_rdtscp (__A);
-}
+#define __rdtscp(a)		__builtin_ia32_rdtscp (a)
 
 #endif /* __iamcu__ */
 
diff --git a/gcc/testsuite/gcc.target/i386/pr99744-1.c b/gcc/testsuite/gcc.target/i386/pr99744-1.c
new file mode 100644
index 00000000000..a5a905c732a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr99744-1.c
@@ -0,0 +1,25 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0" } */
+
+#include <x86intrin.h>
+
+extern unsigned long long int curr_deadline;
+extern void bar (void);
+
+__attribute__ ((target("general-regs-only")))
+void
+foo1 (void)
+{
+  if (__rdtsc () < curr_deadline)
+    return; 
+  bar ();
+}
+
+__attribute__ ((target("general-regs-only")))
+void
+foo2 (unsigned int *p)
+{
+  if (__rdtscp (p) < curr_deadline)
+    return; 
+  bar ();
+}
-- 
2.30.2